[PATCH 0/5] staging: wilc1000: compilation error fixes & cleanup changes

[PATCH 0/5] staging: wilc1000: compilation error fixes & cleanup changes

[Ajay Singh]

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

[PATCH 1/5] staging: wilc1000: avoid arrray of 'wilc_debugfs_info_t' type

[Ajay Singh]

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 <ajay.kathat@microchip.com>
---
 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

Re: [PATCH 1/5] staging: wilc1000: avoid arrray of 'wilc_debugfs_info_t' type

[Greg KH]

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 <ajay.kathat@microchip.com>
> ---
>  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

Re: [PATCH 1/5] staging: wilc1000: avoid arrray of 'wilc_debugfs_info_t' type

[Ajay Singh]

Hi Greg,

On Sun, 12 Aug 2018 13:29:30 +0200
Greg KH <gregkh@linuxfoundation.org> 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 <ajay.kathat@microchip.com>
> > ---
> >  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

Re: [PATCH 1/5] staging: wilc1000: avoid arrray of 'wilc_debugfs_info_t' type

[Greg KH]

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 <gregkh@linuxfoundation.org> 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 <ajay.kathat@microchip.com>
> > > ---
> > >  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

Re: [PATCH 1/5] staging: wilc1000: avoid arrray of 'wilc_debugfs_info_t' type

[Ajay Singh]

Hi Greg,

On Sun, 12 Aug 2018 17:24:21 +0200
Greg KH <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <ajay.kathat@microchip.com>
> > > > ---
> > > >  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

[PATCH 2/5] staging: wilc1000: fixes for undefined reference to `__this_module' error

[Ajay Singh]

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 <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 | 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

Re: [PATCH 2/5] staging: wilc1000: fixes for undefined reference to `__this_module' error

[Greg KH]

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 <lkp@intel.com>
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---

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

Re: [PATCH 2/5] staging: wilc1000: fixes for undefined reference to `__this_module' error

[Ajay Singh]

Hi Greg,

On Sun, 12 Aug 2018 08:42:50 +0200
Greg KH <gregkh@linuxfoundation.org> 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 <lkp@intel.com> Signed-off-by: Ajay Singh
> > <ajay.kathat@microchip.com> ---  
> 
> 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

Re: [PATCH 2/5] staging: wilc1000: fixes for undefined reference to `__this_module' error

[Greg KH]

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 <gregkh@linuxfoundation.org> 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 <lkp@intel.com> Signed-off-by: Ajay Singh
> > > <ajay.kathat@microchip.com> ---  
> > 
> > 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

Re: [PATCH 2/5] staging: wilc1000: fixes for undefined reference to `__this_module' error

[Greg KH]

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 <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 | 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

Re: [PATCH 2/5] staging: wilc1000: fixes for undefined reference to `__this_module' error

[Ajay Singh]

Hi Greg,

On Sun, 12 Aug 2018 13:35:55 +0200
Greg KH <gregkh@linuxfoundation.org> 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 <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 | 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

Re: [PATCH 2/5] staging: wilc1000: fixes for undefined reference to `__this_module' error

[Greg KH]

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 <gregkh@linuxfoundation.org> 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 <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 | 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

Re: [PATCH 2/5] staging: wilc1000: fixes for undefined reference to `__this_module' error

[Dan Carpenter]

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

Re: [PATCH 2/5] staging: wilc1000: fixes for undefined reference to `__this_module' error

[Arend van Spriel]

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

Re: [PATCH 2/5] staging: wilc1000: fixes for undefined reference to `__this_module' error

[Ajay Singh]

Hi Dan,

On Tue, 14 Aug 2018 13:56:21 +0300
Dan Carpenter <dan.carpenter@oracle.com> 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

[PATCH 3/5] staging: wilc1000: remove the use of 'wilc_debugfs_info_t' struct

[Ajay Singh]

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 <ajay.kathat@microchip.com>
---
 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

[PATCH 4/5] staging: wilc1000: change permission to 0600 in debugfs_create_file() call

[Ajay Singh]

Cleanup patch to use more restrictive access permission for debugfs
file.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 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

Re: [PATCH 4/5] staging: wilc1000: change permission to 0600 in debugfs_create_file() call

[Greg KH]

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

[PATCH 5/5] staging: wilc1000: use void return for wilc_debugfs_init()

[Ajay Singh]

Change the return type from int to void for wilc_debugfs_init(), as its
return value is not used.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 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