| Message ID | 1533796986-8832-1-git-send-email-ajay.kathat@microchip.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | staging: wilc1000: fix undefined reference to `__this_module' compilation error | expand |
On Thu, Aug 09, 2018 at 12:13:06PM +0530, Ajay Singh wrote: > wilc_debug.o object file is included for both SDIO and SPI module. When > anyone(either SDIO or SPI) module is compiled as loaded module and another > as buildin module then below compilation error occurs. > > "drivers/staging/wilc1000/wilc_debugfs.o:(.data+0x10): undefined > reference to `__this_module'" > > Moved the declaration of file_operation variable in SDIO/SPI files and > pass this as parameter to wilc_debugfs_init(). > Refactor wilc_debugfs_init() as its not required to maintain > 'wilc_debugfs_info_t' in debugfs_info[] array. Also modified file > permission from 0666 to 0600 & use 'data' field as 'NULL' in > debugfs_create_file() call. > > Fixes: 9abc44ba4e2f("staging: wilc1000: fix TODO to compile spi and sdio > components in single module") > > Reported-by: kbuild test robot <lkp@intel.com> I don't think you need to resend, but the Fixes tag format isn't right. 1) Don't line wrap it. (Maybe checkpatch complains? Ignore checkpatch). 2) Put a space between the git hash and the '('. 3) No blank line between the Fixes and the Reported-by. > -int wilc_debugfs_init(void) > +int wilc_debugfs_init(const struct file_operations *fops) You may as well make wilc_debugfs_init() void since no one checks it. > { > - int i; > - struct wilc_debugfs_info_t *info; > - > wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > - for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) { > - info = &debugfs_info[i]; > - debugfs_create_file(info->name, > - info->perm, > - wilc_dir, > - &info->data, > - &info->fops); > + if (IS_ERR_OR_NULL(wilc_dir)) { > + pr_err("Error creating debugfs\n"); > + return -EFAULT; > } Just check for NULL. If someone builds without debugfs enabled in their .config, that's their choice. No need to print a warning. regards, dan carpenter
On Thu, Aug 09, 2018 at 01:08:38PM +0300, Dan Carpenter wrote: > On Thu, Aug 09, 2018 at 12:13:06PM +0530, Ajay Singh wrote: > > wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > > - for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) { > > - info = &debugfs_info[i]; > > - debugfs_create_file(info->name, > > - info->perm, > > - wilc_dir, > > - &info->data, > > - &info->fops); > > + if (IS_ERR_OR_NULL(wilc_dir)) { > > + pr_err("Error creating debugfs\n"); > > + return -EFAULT; > > } > > Just check for NULL. If someone builds without debugfs enabled in their > .config, that's their choice. No need to print a warning. > Reading it again, I'm not sure my email was clear... Just do this: wilc_dir = debugfs_create_dir("wilc_wifi", NULL); if (!wilc_dir)) { pr_err("Error creating debugfs/wilc_wifi/\n"); return; } If debugfs_create_dir() returns an error pointer it means all the other debugfs functions are also just no-op stub functions. Passing an error pointer to them is harmless. regards, dan carpenter
Hi Dan, On Thu, 9 Aug 2018 13:08:38 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Thu, Aug 09, 2018 at 12:13:06PM +0530, Ajay Singh wrote: > > wilc_debug.o object file is included for both SDIO and SPI module. > > When anyone(either SDIO or SPI) module is compiled as loaded module > > and another as buildin module then below compilation error occurs. > > > > "drivers/staging/wilc1000/wilc_debugfs.o:(.data+0x10): undefined > > reference to `__this_module'" > > > > Moved the declaration of file_operation variable in SDIO/SPI files > > and pass this as parameter to wilc_debugfs_init(). > > Refactor wilc_debugfs_init() as its not required to maintain > > 'wilc_debugfs_info_t' in debugfs_info[] array. Also modified file > > permission from 0666 to 0600 & use 'data' field as 'NULL' in > > debugfs_create_file() call. > > > > Fixes: 9abc44ba4e2f("staging: wilc1000: fix TODO to compile spi and > > sdio components in single module") > > > > Reported-by: kbuild test robot <lkp@intel.com> > > I don't think you need to resend, but the Fixes tag format isn't > right. > > 1) Don't line wrap it. (Maybe checkpatch complains? Ignore > checkpatch). 2) Put a space between the git hash and the '('. > 3) No blank line between the Fixes and the Reported-by. Thanks for your inputs. I will update this commit descriptor and resubmit the patch with correct Fixes tag. > > > -int wilc_debugfs_init(void) > > +int wilc_debugfs_init(const struct file_operations *fops) > > You may as well make wilc_debugfs_init() void since no one checks it. > > > { > > - int i; > > - struct wilc_debugfs_info_t *info; > > - > > wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > > - for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) { > > - info = &debugfs_info[i]; > > - debugfs_create_file(info->name, > > - info->perm, > > - wilc_dir, > > - &info->data, > > - &info->fops); > > + if (IS_ERR_OR_NULL(wilc_dir)) { > > + pr_err("Error creating debugfs\n"); > > + return -EFAULT; > > } > > Just check for NULL. If someone builds without debugfs enabled in > their .config, that's their choice. No need to print a warning. > Sure, I will also include these changes. Regards, Ajay
On Thu, 9 Aug 2018 13:43:58 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Thu, Aug 09, 2018 at 01:08:38PM +0300, Dan Carpenter wrote: > > On Thu, Aug 09, 2018 at 12:13:06PM +0530, Ajay Singh wrote: > > > wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > > > - for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) { > > > - info = &debugfs_info[i]; > > > - debugfs_create_file(info->name, > > > - info->perm, > > > - wilc_dir, > > > - &info->data, > > > - &info->fops); > > > + if (IS_ERR_OR_NULL(wilc_dir)) { > > > + pr_err("Error creating debugfs\n"); > > > + return -EFAULT; > > > } > > > > Just check for NULL. If someone builds without debugfs enabled in > > their .config, that's their choice. No need to print a warning. > > > > Reading it again, I'm not sure my email was clear... Just do this: > > wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > if (!wilc_dir)) { > pr_err("Error creating debugfs/wilc_wifi/\n"); > return; > } > > If debugfs_create_dir() returns an error pointer it means all the > other debugfs functions are also just no-op stub functions. Passing > an error pointer to them is harmless. Thanks for sharing the code snippet. I will include it into v2 patch. Regards, Ajay
On Thu, Aug 09, 2018 at 01:43:58PM +0300, Dan Carpenter wrote: > On Thu, Aug 09, 2018 at 01:08:38PM +0300, Dan Carpenter wrote: > > On Thu, Aug 09, 2018 at 12:13:06PM +0530, Ajay Singh wrote: > > > wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > > > - for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) { > > > - info = &debugfs_info[i]; > > > - debugfs_create_file(info->name, > > > - info->perm, > > > - wilc_dir, > > > - &info->data, > > > - &info->fops); > > > + if (IS_ERR_OR_NULL(wilc_dir)) { > > > + pr_err("Error creating debugfs\n"); > > > + return -EFAULT; > > > } > > > > Just check for NULL. If someone builds without debugfs enabled in their > > .config, that's their choice. No need to print a warning. > > > > Reading it again, I'm not sure my email was clear... Just do this: > > wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > if (!wilc_dir)) { > pr_err("Error creating debugfs/wilc_wifi/\n"); > return; > } > > If debugfs_create_dir() returns an error pointer it means all the other > debugfs functions are also just no-op stub functions. Passing an error > pointer to them is harmless. No, please never care about the return value of a debugfs call, it should never cause your code flow to do anything different. THis should just be: wilc_dir = debugfs_create_dir("wilc_wifi", NULL); and then keep on going. You can always pass the return value of a debugfs call to another one, no need to check anything. I've done a large sweep of the kernel tree for most of this pattern for 4.18, and will keep doing it over time, as it keeps creeping back. greg k-h
On Thu, Aug 09, 2018 at 02:13:24PM +0200, Greg KH wrote: > On Thu, Aug 09, 2018 at 01:43:58PM +0300, Dan Carpenter wrote: > > On Thu, Aug 09, 2018 at 01:08:38PM +0300, Dan Carpenter wrote: > > > On Thu, Aug 09, 2018 at 12:13:06PM +0530, Ajay Singh wrote: > > > > wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > > > > - for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) { > > > > - info = &debugfs_info[i]; > > > > - debugfs_create_file(info->name, > > > > - info->perm, > > > > - wilc_dir, > > > > - &info->data, > > > > - &info->fops); > > > > + if (IS_ERR_OR_NULL(wilc_dir)) { > > > > + pr_err("Error creating debugfs\n"); > > > > + return -EFAULT; > > > > } > > > > > > Just check for NULL. If someone builds without debugfs enabled in their > > > .config, that's their choice. No need to print a warning. > > > > > > > Reading it again, I'm not sure my email was clear... Just do this: > > > > wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > > if (!wilc_dir)) { > > pr_err("Error creating debugfs/wilc_wifi/\n"); > > return; > > } > > > > If debugfs_create_dir() returns an error pointer it means all the other > > debugfs functions are also just no-op stub functions. Passing an error > > pointer to them is harmless. > > No, please never care about the return value of a debugfs call, it > should never cause your code flow to do anything different. THis should > just be: > wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > and then keep on going. You can always pass the return value of a > debugfs call to another one, no need to check anything. > > I've done a large sweep of the kernel tree for most of this pattern for > 4.18, and will keep doing it over time, as it keeps creeping back. Yeah, that's how I thought it was supposed be but then the cleanup doesn't work. We end up putting all the new debugfs files in the base debugsf directory. Doesn't it lead to a use after free if we unload the module? I know that you aren't supposed to unload modules in production. regards, dan carpenter
On Thu, Aug 09, 2018 at 03:51:55PM +0300, Dan Carpenter wrote: > On Thu, Aug 09, 2018 at 02:13:24PM +0200, Greg KH wrote: > > On Thu, Aug 09, 2018 at 01:43:58PM +0300, Dan Carpenter wrote: > > > On Thu, Aug 09, 2018 at 01:08:38PM +0300, Dan Carpenter wrote: > > > > On Thu, Aug 09, 2018 at 12:13:06PM +0530, Ajay Singh wrote: > > > > > wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > > > > > - for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) { > > > > > - info = &debugfs_info[i]; > > > > > - debugfs_create_file(info->name, > > > > > - info->perm, > > > > > - wilc_dir, > > > > > - &info->data, > > > > > - &info->fops); > > > > > + if (IS_ERR_OR_NULL(wilc_dir)) { > > > > > + pr_err("Error creating debugfs\n"); > > > > > + return -EFAULT; > > > > > } > > > > > > > > Just check for NULL. If someone builds without debugfs enabled in their > > > > .config, that's their choice. No need to print a warning. > > > > > > > > > > Reading it again, I'm not sure my email was clear... Just do this: > > > > > > wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > > > if (!wilc_dir)) { > > > pr_err("Error creating debugfs/wilc_wifi/\n"); > > > return; > > > } > > > > > > If debugfs_create_dir() returns an error pointer it means all the other > > > debugfs functions are also just no-op stub functions. Passing an error > > > pointer to them is harmless. > > > > No, please never care about the return value of a debugfs call, it > > should never cause your code flow to do anything different. THis should > > just be: > > wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > > and then keep on going. You can always pass the return value of a > > debugfs call to another one, no need to check anything. > > > > I've done a large sweep of the kernel tree for most of this pattern for > > 4.18, and will keep doing it over time, as it keeps creeping back. > > Yeah, that's how I thought it was supposed be but then the cleanup > doesn't work. We end up putting all the new debugfs files in the base > debugsf directory. Doesn't it lead to a use after free if we unload the > module? If the creation of a directory fails, then almost always the creation of files will also fail, so no real problem here as your system is really hosed at that point in time. :) thanks, greg k-h
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c index 01cf4bd..37885ad 100644 --- a/drivers/staging/wilc1000/linux_wlan.c +++ b/drivers/staging/wilc1000/linux_wlan.c @@ -1062,7 +1062,6 @@ int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type, if (!wl) return -ENOMEM; - wilc_debugfs_init(); *wilc = wl; wl->io_type = io_type; wl->hif_func = ops; diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c index edc7287..cb02859 100644 --- a/drivers/staging/wilc1000/wilc_debugfs.c +++ b/drivers/staging/wilc1000/wilc_debugfs.c @@ -20,8 +20,8 @@ static struct dentry *wilc_dir; #define DBG_LEVEL_ALL (DEBUG | INFO | WRN | ERR) static atomic_t WILC_DEBUG_LEVEL = ATOMIC_INIT(ERR); -static ssize_t wilc_debug_level_read(struct file *file, char __user *userbuf, - size_t count, loff_t *ppos) +ssize_t wilc_debug_level_read(struct file *file, char __user *userbuf, + size_t count, loff_t *ppos) { char buf[128]; int res = 0; @@ -36,9 +36,8 @@ static ssize_t wilc_debug_level_read(struct file *file, char __user *userbuf, return simple_read_from_buffer(userbuf, count, ppos, buf, res); } -static ssize_t wilc_debug_level_write(struct file *filp, - const char __user *buf, size_t count, - loff_t *ppos) +ssize_t wilc_debug_level_write(struct file *filp, const char __user *buf, + size_t count, loff_t *ppos) { int flag = 0; int ret; @@ -63,44 +62,15 @@ static ssize_t wilc_debug_level_write(struct file *filp, return count; } -#define FOPS(_open, _read, _write, _poll) { \ - .owner = THIS_MODULE, \ - .open = (_open), \ - .read = (_read), \ - .write = (_write), \ - .poll = (_poll), \ -} - -struct wilc_debugfs_info_t { - const char *name; - int perm; - unsigned int data; - const struct file_operations fops; -}; - -static struct wilc_debugfs_info_t debugfs_info[] = { - { - "wilc_debug_level", - 0666, - (DEBUG | ERR), - FOPS(NULL, wilc_debug_level_read, wilc_debug_level_write, NULL), - }, -}; - -int wilc_debugfs_init(void) +int wilc_debugfs_init(const struct file_operations *fops) { - int i; - struct wilc_debugfs_info_t *info; - wilc_dir = debugfs_create_dir("wilc_wifi", NULL); - for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) { - info = &debugfs_info[i]; - debugfs_create_file(info->name, - info->perm, - wilc_dir, - &info->data, - &info->fops); + if (IS_ERR_OR_NULL(wilc_dir)) { + pr_err("Error creating debugfs\n"); + return -EFAULT; } + debugfs_create_file("wilc_debug_level", 0600, wilc_dir, NULL, fops); + return 0; } diff --git a/drivers/staging/wilc1000/wilc_sdio.c b/drivers/staging/wilc1000/wilc_sdio.c index b2080d8..c1573f5 100644 --- a/drivers/staging/wilc1000/wilc_sdio.c +++ b/drivers/staging/wilc1000/wilc_sdio.c @@ -103,6 +103,12 @@ static int wilc_sdio_cmd53(struct wilc *wilc, struct sdio_cmd53 *cmd) return ret; } +static const struct file_operations sdio_debug_fops = { + .owner = THIS_MODULE, + .read = wilc_debug_level_read, + .write = wilc_debug_level_write, +}; + static int linux_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id) { @@ -126,6 +132,7 @@ static int linux_sdio_probe(struct sdio_func *func, dev_err(&func->dev, "Couldn't initialize netdev\n"); return ret; } + wilc_debugfs_init(&sdio_debug_fops); sdio_set_drvdata(func, wilc); wilc->dev = &func->dev; wilc->gpio_irq = gpio; diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c index 5517477..0376649 100644 --- a/drivers/staging/wilc1000/wilc_spi.c +++ b/drivers/staging/wilc1000/wilc_spi.c @@ -102,6 +102,12 @@ static u8 crc7(u8 crc, const u8 *buffer, u32 len) #define USE_SPI_DMA 0 +static const struct file_operations spi_debug_fops = { + .owner = THIS_MODULE, + .read = wilc_debug_level_read, + .write = wilc_debug_level_write, +}; + static int wilc_bus_probe(struct spi_device *spi) { int ret; @@ -120,6 +126,7 @@ static int wilc_bus_probe(struct spi_device *spi) if (ret) return ret; + wilc_debugfs_init(&spi_debug_fops); spi_set_drvdata(spi, wilc); wilc->dev = &spi->dev; wilc->gpio_irq = gpio; diff --git a/drivers/staging/wilc1000/wilc_wlan_if.h b/drivers/staging/wilc1000/wilc_wlan_if.h index 00d13b1..47b4020 100644 --- a/drivers/staging/wilc1000/wilc_wlan_if.h +++ b/drivers/staging/wilc1000/wilc_wlan_if.h @@ -831,6 +831,11 @@ struct wilc; int wilc_wlan_init(struct net_device *dev); u32 wilc_get_chipid(struct wilc *wilc, bool update); -int wilc_debugfs_init(void); +int wilc_debugfs_init(const struct file_operations *fops); void wilc_debugfs_remove(void); +ssize_t wilc_debug_level_read(struct file *file, char __user *userbuf, + size_t count, loff_t *ppos); +ssize_t wilc_debug_level_write(struct file *filp, const char __user *buf, + size_t count, loff_t *ppos); + #endif
wilc_debug.o object file is included for both SDIO and SPI module. When anyone(either SDIO or SPI) module is compiled as loaded module and another as buildin module then below compilation error occurs. "drivers/staging/wilc1000/wilc_debugfs.o:(.data+0x10): undefined reference to `__this_module'" Moved the declaration of file_operation variable in SDIO/SPI files and pass this as parameter to wilc_debugfs_init(). Refactor wilc_debugfs_init() as its not required to maintain 'wilc_debugfs_info_t' in debugfs_info[] array. Also modified file permission from 0666 to 0600 & use 'data' field as 'NULL' in debugfs_create_file() call. Fixes: 9abc44ba4e2f("staging: wilc1000: fix TODO to compile spi and sdio components in single module") Reported-by: kbuild test robot <lkp@intel.com> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> --- drivers/staging/wilc1000/linux_wlan.c | 1 - drivers/staging/wilc1000/wilc_debugfs.c | 50 +++++++-------------------------- drivers/staging/wilc1000/wilc_sdio.c | 7 +++++ drivers/staging/wilc1000/wilc_spi.c | 7 +++++ drivers/staging/wilc1000/wilc_wlan_if.h | 7 ++++- 5 files changed, 30 insertions(+), 42 deletions(-)