This patch series contains the changes to avoid below compilation error.
"drivers/staging/wilc1000/wilc_debugfs.o:(.data+0x10): undefined
reference to `__this_module'"
Split the changes into multiple patches, as discussed in [1].
[1]. https://www.spinics.net/lists/linux-wireless/msg176009.html
Ajay Singh (5):
staging: wilc1000: avoid arrray of 'wilc_debugfs_info_t' type
staging: wilc1000: fixes for undefined reference to `__this_module'
error
staging: wilc1000: remove the use of 'wilc_debugfs_info_t' struct
staging: wilc1000: change permission to 0600 in debugfs_create_file()
call
staging: wilc1000: use void return for wilc_debugfs_init()
drivers/staging/wilc1000/linux_wlan.c | 1 -
drivers/staging/wilc1000/wilc_debugfs.c | 48 +++++----------------------------
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, 26 insertions(+), 44 deletions(-)
--
2.7.4
Hi Greg,
On Sun, 12 Aug 2018 13:35:55 +0200
Greg KH <[email protected]> wrote:
> On Sun, Aug 12, 2018 at 10:17:42AM +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(). Remove 'fops'
> > element from 'wilc_debugfs_info_t' struct, as its not required.
> >
> > Fixes: 9abc44ba4e2f ("staging: wilc1000: fix TODO to compile spi
> > and sdio components in single module") Reported-by: kbuild test
> > robot <[email protected]> Signed-off-by: Ajay Singh
> > <[email protected]> ---
> > drivers/staging/wilc1000/linux_wlan.c | 1 -
> > drivers/staging/wilc1000/wilc_debugfs.c | 23
> > ++++++----------------- 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, 26 insertions(+), 19 deletions(-)
> >
> > 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 c5f67c9..78c0831
> > 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,35 +62,25 @@ 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)
> > {
> > struct wilc_debugfs_info_t *info = &debugfs_info;
> >
> > wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
> > debugfs_create_file(info->name, info->perm, wilc_dir,
> > &info->data,
> > - &info->fops);
> > + 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,
> > +};
>
> Wait, step back a second. Why all of this work for a debugfs file
> that no one uses? In wilc_debugfs.c there seems to only provide a
> single debugfs file to read and write a value that no one uses
> anywhere.
>
> So why not a simple patch to just delete the wilc_debugfs.c file
> entirely? Who even uses this debugfs file? And if they do, what is
> it for? What am I missing about how this is used?
Yes, currently only code to read and writing for "wilc_debug_level"
exists.
The main purpose for this file was to configure(enable/disable)
specific level debug logs to print.
Previously there were custom debug print API's like PRINT_K,
PRINT_INFO etc. Those portion of code is cleanup in previous patches
now only debugfs file is remaining.
As of now this file is not used, but we had plans to use this
setting to enable/disable specific the debug prints. After completing
the cleanup activity for driver we had plans to take it up.
Regards,
Ajay
On Sun, Aug 12, 2018 at 05:48:52PM +0530, Ajay Singh wrote:
> Hi Greg,
>
> On Sun, 12 Aug 2018 13:35:55 +0200
> Greg KH <[email protected]> wrote:
>
> > On Sun, Aug 12, 2018 at 10:17:42AM +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(). Remove 'fops'
> > > element from 'wilc_debugfs_info_t' struct, as its not required.
> > >
> > > Fixes: 9abc44ba4e2f ("staging: wilc1000: fix TODO to compile spi
> > > and sdio components in single module") Reported-by: kbuild test
> > > robot <[email protected]> Signed-off-by: Ajay Singh
> > > <[email protected]> ---
> > > drivers/staging/wilc1000/linux_wlan.c | 1 -
> > > drivers/staging/wilc1000/wilc_debugfs.c | 23
> > > ++++++----------------- 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, 26 insertions(+), 19 deletions(-)
> > >
> > > 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 c5f67c9..78c0831
> > > 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,35 +62,25 @@ 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)
> > > {
> > > struct wilc_debugfs_info_t *info = &debugfs_info;
> > >
> > > wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
> > > debugfs_create_file(info->name, info->perm, wilc_dir,
> > > &info->data,
> > > - &info->fops);
> > > + 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,
> > > +};
> >
> > Wait, step back a second. Why all of this work for a debugfs file
> > that no one uses? In wilc_debugfs.c there seems to only provide a
> > single debugfs file to read and write a value that no one uses
> > anywhere.
> >
> > So why not a simple patch to just delete the wilc_debugfs.c file
> > entirely? Who even uses this debugfs file? And if they do, what is
> > it for? What am I missing about how this is used?
>
>
> Yes, currently only code to read and writing for "wilc_debug_level"
> exists.
>
> The main purpose for this file was to configure(enable/disable)
> specific level debug logs to print.
>
> Previously there were custom debug print API's like PRINT_K,
> PRINT_INFO etc. Those portion of code is cleanup in previous patches
> now only debugfs file is remaining.
>
> As of now this file is not used, but we had plans to use this
> setting to enable/disable specific the debug prints. After completing
> the cleanup activity for driver we had plans to take it up.
No, please no. Let's delete it now, you do not have a custom
api/interface for controlling logging levels of a single driver that is
separate from the entire rest of the kernel. Use what everyone else
uses, the default interfaces we have and all use.
And we do not leave code around in the kernel "to be used in the
future", if it is not used now, it gets deleted. You can easily add it
back later if you really need it, that's the beauty of git :)
thanks,
greg k-h
On Sun, Aug 12, 2018 at 10:17:42AM +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(). Remove 'fops' element
> from 'wilc_debugfs_info_t' struct, as its not required.
>
> Fixes: 9abc44ba4e2f ("staging: wilc1000: fix TODO to compile spi and sdio components in single module")
> Reported-by: kbuild test robot <[email protected]>
> Signed-off-by: Ajay Singh <[email protected]>
> ---
> drivers/staging/wilc1000/linux_wlan.c | 1 -
> drivers/staging/wilc1000/wilc_debugfs.c | 23 ++++++-----------------
> 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, 26 insertions(+), 19 deletions(-)
>
> 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 c5f67c9..78c0831 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,35 +62,25 @@ 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)
> {
> struct wilc_debugfs_info_t *info = &debugfs_info;
>
> wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
> debugfs_create_file(info->name, info->perm, wilc_dir, &info->data,
> - &info->fops);
> + 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,
> +};
Wait, step back a second. Why all of this work for a debugfs file that
no one uses? In wilc_debugfs.c there seems to only provide a single
debugfs file to read and write a value that no one uses anywhere.
So why not a simple patch to just delete the wilc_debugfs.c file
entirely? Who even uses this debugfs file? And if they do, what is it
for? What am I missing about how this is used?
thanks,
greg k-h
On Sun, Aug 12, 2018 at 10:17:44AM +0530, Ajay Singh wrote:
> Cleanup patch to use more restrictive access permission for debugfs
> file.
Why? What is the harm in writing to a value that is never used? :)
thanks,
greg k-h
Hi Dan,
On Tue, 14 Aug 2018 13:56:21 +0300
Dan Carpenter <[email protected]> wrote:
> On Sun, Aug 12, 2018 at 05:48:52PM +0530, Ajay Singh wrote:
> > Yes, currently only code to read and writing for "wilc_debug_level"
> > exists.
> >
> > The main purpose for this file was to configure(enable/disable)
> > specific level debug logs to print.
> >
>
> The kernel's dev_dbg() macros are really very clever. You can enable
> them for one file or for a specific line or for a function.
>
Thanks for the information.
I will explore these debug macros in future.
Regards,
Ajay
Refactor the code to remove the use of 'wilc_debugfs_info_t' struct.
Pass NULL for data in debugfs_create_file() call, as its value is not
used.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/staging/wilc1000/wilc_debugfs.c | 17 +----------------
1 file changed, 1 insertion(+), 16 deletions(-)
diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c
index 78c0831..c9bc325 100644
--- a/drivers/staging/wilc1000/wilc_debugfs.c
+++ b/drivers/staging/wilc1000/wilc_debugfs.c
@@ -62,25 +62,10 @@ ssize_t wilc_debug_level_write(struct file *filp, const char __user *buf,
return count;
}
-struct wilc_debugfs_info_t {
- const char *name;
- int perm;
- unsigned int data;
-};
-
-static struct wilc_debugfs_info_t debugfs_info = {
- "wilc_debug_level",
- 0666,
- (DEBUG | ERR),
-};
-
int wilc_debugfs_init(const struct file_operations *fops)
{
- struct wilc_debugfs_info_t *info = &debugfs_info;
-
wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
- debugfs_create_file(info->name, info->perm, wilc_dir, &info->data,
- fops);
+ debugfs_create_file("wilc_debug_level", 0666, wilc_dir, NULL, fops);
return 0;
}
--
2.7.4
Hi Greg,
On Sun, 12 Aug 2018 08:42:50 +0200
Greg KH <[email protected]> wrote:
> On Sun, Aug 12, 2018 at 10:17:42AM +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(). Remove 'fops'
> > element from 'wilc_debugfs_info_t' struct, as its not required.
> >
> > Fixes: 9abc44ba4e2f ("staging: wilc1000: fix TODO to compile spi
> > and sdio components in single module") Reported-by: kbuild test
> > robot <[email protected]> Signed-off-by: Ajay Singh
> > <[email protected]> ---
>
> Can this be the first patch in the series so it can go to Linus for
> 4.19 and the rest can be postponed for later?
>
No, the patches are in order. To apply this patch the first patch has
to be also applied.
If its okay, I can merge the first 2 patches and give a single patch
so it can be sent for 4.19 release. Rest of the patches can be
postponed for later.
Regards,
Ajay
On Sun, Aug 12, 2018 at 05:48:52PM +0530, Ajay Singh wrote:
> Yes, currently only code to read and writing for "wilc_debug_level"
> exists.
>
> The main purpose for this file was to configure(enable/disable)
> specific level debug logs to print.
>
The kernel's dev_dbg() macros are really very clever. You can enable
them for one file or for a specific line or for a function.
regards,
dan carpenter
On 8/14/2018 12:56 PM, Dan Carpenter wrote:
> On Sun, Aug 12, 2018 at 05:48:52PM +0530, Ajay Singh wrote:
>> Yes, currently only code to read and writing for "wilc_debug_level"
>> exists.
>>
>> The main purpose for this file was to configure(enable/disable)
>> specific level debug logs to print.
>>
>
> The kernel's dev_dbg() macros are really very clever. You can enable
> them for one file or for a specific line or for a function.
Provided you have CONFIG_DYNAMIC_DEBUG selected.
Gr. AvS
On Sun, Aug 12, 2018 at 05:18:29PM +0530, Ajay Singh wrote:
> Hi Greg,
>
> On Sun, 12 Aug 2018 13:29:30 +0200
> Greg KH <[email protected]> wrote:
>
> > On Sun, Aug 12, 2018 at 10:17:41AM +0530, Ajay Singh wrote:
> > > Refactor code by removing array of 'wilc_debugfs_info_t' type and
> > > use single variable to store 'wilc_debugfs_info_t' struct value.
> > >
> > > Signed-off-by: Ajay Singh <[email protected]>
> > > ---
> > > drivers/staging/wilc1000/wilc_debugfs.c | 26
> > > +++++++++----------------- 1 file changed, 9 insertions(+), 17
> > > deletions(-)
> > >
> > > diff --git a/drivers/staging/wilc1000/wilc_debugfs.c
> > > b/drivers/staging/wilc1000/wilc_debugfs.c index edc7287..c5f67c9
> > > 100644 --- a/drivers/staging/wilc1000/wilc_debugfs.c
> > > +++ b/drivers/staging/wilc1000/wilc_debugfs.c
> > > @@ -78,29 +78,21 @@ struct wilc_debugfs_info_t {
> > > 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),
> > > - },
> > > +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 i;
> > > - struct wilc_debugfs_info_t *info;
> > > + struct wilc_debugfs_info_t *info = &debugfs_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);
> > > - }
> > > + debugfs_create_file(info->name, info->perm, wilc_dir,
> > > &info->data,
> > > + &info->fops);
> > > +
> >
> > Why go through the extra step here in changing this variable from an
> > array to a single value and not just make the call to
> > debugfs_create_file() directly with the individual values like you do
> > in patch 3? It feels like this step isn't needed at all here, right?
> >
> > What am I missing?
>
> Actually, while calling debugfs_create_file() it also passed the
> "info->data", "info->perm" which was maintained in a static variable.
> So to have the changes in multiple patches, I first tried by removing
> the dependency of array and later changed the individual parameters
> for this debugfs_create_file().
But you don't need/want that here anyway, it would have been simpler to
just do it correct in one patch. That would have also been easier to
review :)
But for now, just delete this file as it is not used at all. Want me to
do it for you?
thanks,
greg k-h
Refactor code by removing array of 'wilc_debugfs_info_t' type and use
single variable to store 'wilc_debugfs_info_t' struct value.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/staging/wilc1000/wilc_debugfs.c | 26 +++++++++-----------------
1 file changed, 9 insertions(+), 17 deletions(-)
diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c
index edc7287..c5f67c9 100644
--- a/drivers/staging/wilc1000/wilc_debugfs.c
+++ b/drivers/staging/wilc1000/wilc_debugfs.c
@@ -78,29 +78,21 @@ struct wilc_debugfs_info_t {
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),
- },
+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 i;
- struct wilc_debugfs_info_t *info;
+ struct wilc_debugfs_info_t *info = &debugfs_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);
- }
+ debugfs_create_file(info->name, info->perm, wilc_dir, &info->data,
+ &info->fops);
+
return 0;
}
--
2.7.4
On Sun, Aug 12, 2018 at 10:17:41AM +0530, Ajay Singh wrote:
> Refactor code by removing array of 'wilc_debugfs_info_t' type and use
> single variable to store 'wilc_debugfs_info_t' struct value.
>
> Signed-off-by: Ajay Singh <[email protected]>
> ---
> drivers/staging/wilc1000/wilc_debugfs.c | 26 +++++++++-----------------
> 1 file changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c
> index edc7287..c5f67c9 100644
> --- a/drivers/staging/wilc1000/wilc_debugfs.c
> +++ b/drivers/staging/wilc1000/wilc_debugfs.c
> @@ -78,29 +78,21 @@ struct wilc_debugfs_info_t {
> 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),
> - },
> +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 i;
> - struct wilc_debugfs_info_t *info;
> + struct wilc_debugfs_info_t *info = &debugfs_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);
> - }
> + debugfs_create_file(info->name, info->perm, wilc_dir, &info->data,
> + &info->fops);
> +
Why go through the extra step here in changing this variable from an
array to a single value and not just make the call to
debugfs_create_file() directly with the individual values like you do in
patch 3? It feels like this step isn't needed at all here, right?
What am I missing?
thanks,
greg k-h
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(). Remove 'fops' element
from 'wilc_debugfs_info_t' struct, as its not required.
Fixes: 9abc44ba4e2f ("staging: wilc1000: fix TODO to compile spi and sdio components in single module")
Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/staging/wilc1000/linux_wlan.c | 1 -
drivers/staging/wilc1000/wilc_debugfs.c | 23 ++++++-----------------
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, 26 insertions(+), 19 deletions(-)
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 c5f67c9..78c0831 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,35 +62,25 @@ 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)
{
struct wilc_debugfs_info_t *info = &debugfs_info;
wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
debugfs_create_file(info->name, info->perm, wilc_dir, &info->data,
- &info->fops);
+ 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
--
2.7.4
Cleanup patch to use more restrictive access permission for debugfs
file.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/staging/wilc1000/wilc_debugfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c
index c9bc325..15e797b 100644
--- a/drivers/staging/wilc1000/wilc_debugfs.c
+++ b/drivers/staging/wilc1000/wilc_debugfs.c
@@ -65,7 +65,7 @@ ssize_t wilc_debug_level_write(struct file *filp, const char __user *buf,
int wilc_debugfs_init(const struct file_operations *fops)
{
wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
- debugfs_create_file("wilc_debug_level", 0666, wilc_dir, NULL, fops);
+ debugfs_create_file("wilc_debug_level", 0600, wilc_dir, NULL, fops);
return 0;
}
--
2.7.4
Change the return type from int to void for wilc_debugfs_init(), as its
return value is not used.
Signed-off-by: Ajay Singh <[email protected]>
---
drivers/staging/wilc1000/wilc_debugfs.c | 4 +---
drivers/staging/wilc1000/wilc_wlan_if.h | 2 +-
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c
index 15e797b..1a2c4e7 100644
--- a/drivers/staging/wilc1000/wilc_debugfs.c
+++ b/drivers/staging/wilc1000/wilc_debugfs.c
@@ -62,12 +62,10 @@ ssize_t wilc_debug_level_write(struct file *filp, const char __user *buf,
return count;
}
-int wilc_debugfs_init(const struct file_operations *fops)
+void wilc_debugfs_init(const struct file_operations *fops)
{
wilc_dir = debugfs_create_dir("wilc_wifi", NULL);
debugfs_create_file("wilc_debug_level", 0600, wilc_dir, NULL, fops);
-
- return 0;
}
void wilc_debugfs_remove(void)
diff --git a/drivers/staging/wilc1000/wilc_wlan_if.h b/drivers/staging/wilc1000/wilc_wlan_if.h
index 47b4020..21f72a8 100644
--- a/drivers/staging/wilc1000/wilc_wlan_if.h
+++ b/drivers/staging/wilc1000/wilc_wlan_if.h
@@ -831,7 +831,7 @@ struct wilc;
int wilc_wlan_init(struct net_device *dev);
u32 wilc_get_chipid(struct wilc *wilc, bool update);
-int wilc_debugfs_init(const struct file_operations *fops);
+void 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);
--
2.7.4
On Sun, Aug 12, 2018 at 10:17:42AM +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(). Remove 'fops' element
> from 'wilc_debugfs_info_t' struct, as its not required.
>
> Fixes: 9abc44ba4e2f ("staging: wilc1000: fix TODO to compile spi and sdio components in single module")
> Reported-by: kbuild test robot <[email protected]>
> Signed-off-by: Ajay Singh <[email protected]>
> ---
Can this be the first patch in the series so it can go to Linus for 4.19
and the rest can be postponed for later?
thanks,
greg k-h
Hi Greg,
On Sun, 12 Aug 2018 17:24:21 +0200
Greg KH <[email protected]> wrote:
> On Sun, Aug 12, 2018 at 05:18:29PM +0530, Ajay Singh wrote:
> > Hi Greg,
> >
> > On Sun, 12 Aug 2018 13:29:30 +0200
> > Greg KH <[email protected]> wrote:
> >
> > > On Sun, Aug 12, 2018 at 10:17:41AM +0530, Ajay Singh wrote:
> > > > Refactor code by removing array of 'wilc_debugfs_info_t' type
> > > > and use single variable to store 'wilc_debugfs_info_t' struct
> > > > value.
> > > >
> > > > Signed-off-by: Ajay Singh <[email protected]>
> > > > ---
> > > > drivers/staging/wilc1000/wilc_debugfs.c | 26
> > > > +++++++++----------------- 1 file changed, 9 insertions(+), 17
> > > > deletions(-)
> > > >
> > > > diff --git a/drivers/staging/wilc1000/wilc_debugfs.c
> > > > b/drivers/staging/wilc1000/wilc_debugfs.c index edc7287..c5f67c9
> > > > 100644 --- a/drivers/staging/wilc1000/wilc_debugfs.c
> > > > +++ b/drivers/staging/wilc1000/wilc_debugfs.c
> > > > @@ -78,29 +78,21 @@ struct wilc_debugfs_info_t {
> > > > 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),
> > > > - },
> > > > +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 i;
> > > > - struct wilc_debugfs_info_t *info;
> > > > + struct wilc_debugfs_info_t *info = &debugfs_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);
> > > > - }
> > > > + debugfs_create_file(info->name, info->perm, wilc_dir,
> > > > &info->data,
> > > > + &info->fops);
> > > > +
> > >
> > > Why go through the extra step here in changing this variable from
> > > an array to a single value and not just make the call to
> > > debugfs_create_file() directly with the individual values like
> > > you do in patch 3? It feels like this step isn't needed at all
> > > here, right?
> > >
> > > What am I missing?
> >
> > Actually, while calling debugfs_create_file() it also passed the
> > "info->data", "info->perm" which was maintained in a static
> > variable. So to have the changes in multiple patches, I first tried
> > by removing the dependency of array and later changed the
> > individual parameters for this debugfs_create_file().
>
> But you don't need/want that here anyway, it would have been simpler
> to just do it correct in one patch. That would have also been easier
> to review :)
>
> But for now, just delete this file as it is not used at all. Want me
> to do it for you?
>
Thanks for giving your time and support.
I will make the changes and submit the patch to delete wilc_debugfs.c
file.
Regards,
Ajay
On Sun, Aug 12, 2018 at 04:52:04PM +0530, Ajay Singh wrote:
> Hi Greg,
>
> On Sun, 12 Aug 2018 08:42:50 +0200
> Greg KH <[email protected]> wrote:
>
> > On Sun, Aug 12, 2018 at 10:17:42AM +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(). Remove 'fops'
> > > element from 'wilc_debugfs_info_t' struct, as its not required.
> > >
> > > Fixes: 9abc44ba4e2f ("staging: wilc1000: fix TODO to compile spi
> > > and sdio components in single module") Reported-by: kbuild test
> > > robot <[email protected]> Signed-off-by: Ajay Singh
> > > <[email protected]> ---
> >
> > Can this be the first patch in the series so it can go to Linus for
> > 4.19 and the rest can be postponed for later?
> >
>
> No, the patches are in order. To apply this patch the first patch has
> to be also applied.
>
> If its okay, I can merge the first 2 patches and give a single patch
> so it can be sent for 4.19 release. Rest of the patches can be
> postponed for later.
No, no need for merging them, I'll just split them up that way then, I
was just hoping for a simpler path toward getting this fixed. I'll work
on this after 4.19-rc1 is out.
thanks,
greg k-h
Hi Greg,
On Sun, 12 Aug 2018 13:29:30 +0200
Greg KH <[email protected]> wrote:
> On Sun, Aug 12, 2018 at 10:17:41AM +0530, Ajay Singh wrote:
> > Refactor code by removing array of 'wilc_debugfs_info_t' type and
> > use single variable to store 'wilc_debugfs_info_t' struct value.
> >
> > Signed-off-by: Ajay Singh <[email protected]>
> > ---
> > drivers/staging/wilc1000/wilc_debugfs.c | 26
> > +++++++++----------------- 1 file changed, 9 insertions(+), 17
> > deletions(-)
> >
> > diff --git a/drivers/staging/wilc1000/wilc_debugfs.c
> > b/drivers/staging/wilc1000/wilc_debugfs.c index edc7287..c5f67c9
> > 100644 --- a/drivers/staging/wilc1000/wilc_debugfs.c
> > +++ b/drivers/staging/wilc1000/wilc_debugfs.c
> > @@ -78,29 +78,21 @@ struct wilc_debugfs_info_t {
> > 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),
> > - },
> > +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 i;
> > - struct wilc_debugfs_info_t *info;
> > + struct wilc_debugfs_info_t *info = &debugfs_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);
> > - }
> > + debugfs_create_file(info->name, info->perm, wilc_dir,
> > &info->data,
> > + &info->fops);
> > +
>
> Why go through the extra step here in changing this variable from an
> array to a single value and not just make the call to
> debugfs_create_file() directly with the individual values like you do
> in patch 3? It feels like this step isn't needed at all here, right?
>
> What am I missing?
Actually, while calling debugfs_create_file() it also passed the
"info->data", "info->perm" which was maintained in a static variable.
So to have the changes in multiple patches, I first tried by removing
the dependency of array and later changed the individual parameters
for this debugfs_create_file().
Regards
Ajay