On Thu, May 07, 2020 at 05:27:38PM -0700, Scott Branden wrote:
+#if defined(CONFIG_REQ_FW_INTO_BUF_PRIV)
+#define KERNEL_PREAD_FLAG_PART 0x0001 /* Allow reading part of file */ +#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf_priv +int request_firmware_into_buf_priv(const struct firmware **firmware_p,
const char *name, struct device *device,void *buf, size_t size,size_t offset, unsigned int pread_flags);+#else
+#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf
+#endif
+#endif
Please clean this up, the code must reflect only the code upstream. No config stuff like this should be used on the driver. I had to stop my review here.
Luis
Hi Luis,
On 2020-05-12 5:38 p.m., Luis Chamberlain wrote:
On Thu, May 07, 2020 at 05:27:38PM -0700, Scott Branden wrote:
+#if defined(CONFIG_REQ_FW_INTO_BUF_PRIV)
+#define KERNEL_PREAD_FLAG_PART 0x0001 /* Allow reading part of file */ +#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf_priv +int request_firmware_into_buf_priv(const struct firmware **firmware_p,
const char *name, struct device *device,void *buf, size_t size,size_t offset, unsigned int pread_flags);+#else
+#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf
+#endif
+#endif
Please clean this up, the code must reflect only the code upstream. No config stuff like this should be used on the driver. I had to stop my review here.
The CONFIG_ prefix shouldn't have been there as there is no Kconfig option to select this. Would like to just change it to a normal define without CONFIG_ prefix instead? This code is here to allow a limited version of the driver to run on older kernels which do not have the necessary partial file read support. By having it in the upstream codebase we don't need to maintain an internal version of the driver. User can take the upstream kernel module and compile it against an old version of the kernel by via the define.
Luis
Regards, Scott
On Tue, May 12, 2020 at 11:31:28PM -0700, Scott Branden wrote:
Hi Luis,
On 2020-05-12 5:38 p.m., Luis Chamberlain wrote:
On Thu, May 07, 2020 at 05:27:38PM -0700, Scott Branden wrote:
+#if defined(CONFIG_REQ_FW_INTO_BUF_PRIV)
+#define KERNEL_PREAD_FLAG_PART 0x0001 /* Allow reading part of file */ +#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf_priv +int request_firmware_into_buf_priv(const struct firmware **firmware_p,
const char *name, struct device *device,void *buf, size_t size,size_t offset, unsigned int pread_flags);+#else
+#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf
+#endif
+#endif
Please clean this up, the code must reflect only the code upstream. No config stuff like this should be used on the driver. I had to stop my review here.
The CONFIG_ prefix shouldn't have been there as there is no Kconfig option to select this. Would like to just change it to a normal define without CONFIG_ prefix instead? This code is here to allow a limited version of the driver to run on older kernels which do not have the necessary partial file read support. By having it in the upstream codebase we don't need to maintain an internal version of the driver. User can take the upstream kernel module and compile it against an old version of the kernel by via the define.
That's not how kernel drivers in the tree work, sorry. They do not contain "older kernel support" in them, they work as a whole with the rest of the kernel they ship with only.
Otherwise all drivers would be a total mess over time, can you imagine doing this for the next 20+ years? Not maintainable.
thanks,
greg k-h
On Wed, May 13, 2020 at 08:50:46AM +0200, Greg Kroah-Hartman wrote:
On Tue, May 12, 2020 at 11:31:28PM -0700, Scott Branden wrote:
Hi Luis,
On 2020-05-12 5:38 p.m., Luis Chamberlain wrote:
On Thu, May 07, 2020 at 05:27:38PM -0700, Scott Branden wrote:
+#if defined(CONFIG_REQ_FW_INTO_BUF_PRIV)
+#define KERNEL_PREAD_FLAG_PART 0x0001 /* Allow reading part of file */ +#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf_priv +int request_firmware_into_buf_priv(const struct firmware **firmware_p,
const char *name, struct device *device,void *buf, size_t size,size_t offset, unsigned int pread_flags);+#else
+#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf
+#endif
+#endif
Please clean this up, the code must reflect only the code upstream. No config stuff like this should be used on the driver. I had to stop my review here.
The CONFIG_ prefix shouldn't have been there as there is no Kconfig option to select this. Would like to just change it to a normal define without CONFIG_ prefix instead? This code is here to allow a limited version of the driver to run on older kernels which do not have the necessary partial file read support. By having it in the upstream codebase we don't need to maintain an internal version of the driver. User can take the upstream kernel module and compile it against an old version of the kernel by via the define.
That's not how kernel drivers in the tree work, sorry. They do not contain "older kernel support" in them, they work as a whole with the rest of the kernel they ship with only.
Otherwise all drivers would be a total mess over time, can you imagine doing this for the next 20+ years? Not maintainable.
Scott, now imagine the amount of cleanup you'd need to do to your driver to get it to a state where it doesn't depend on any old kernel. That's the exact shape of the driver we want.
To backport, you can look into the backports project which strives to backport drivers automatically [0] to older kernels.
[0] https://backports.wiki.kernel.org/index.php/Main_Page
Luis
Thanks for link.
On 2020-05-13 5:30 a.m., Luis Chamberlain wrote:
On Wed, May 13, 2020 at 08:50:46AM +0200, Greg Kroah-Hartman wrote:
On Tue, May 12, 2020 at 11:31:28PM -0700, Scott Branden wrote:
That's not how kernel drivers in the tree work, sorry. They do not contain "older kernel support" in them, they work as a whole with the rest of the kernel they ship with only.
Otherwise all drivers would be a total mess over time, can you imagine doing this for the next 20+ years? Not maintainable.
Scott, now imagine the amount of cleanup you'd need to do to your driver to get it to a state where it doesn't depend on any old kernel. That's the exact shape of the driver we want.
To backport, you can look into the backports project which strives to backport drivers automatically [0] to older kernels.
[0] https://backports.wiki.kernel.org/index.php/Main_Page
Luis
Will drop legacy support from patch and look closer at this.
Thanks, Scott
linux-kselftest-mirror@lists.linaro.org
-
Greg Kroah-Hartman -
Luis Chamberlain -
Scott Branden