[PATCH] stmmac: fix sparse warnings

[PATCH] stmmac: fix sparse warnings

[Andy Shevchenko]

This patch fixes the following sparse warnings.

drivers/net/ethernet/stmicro/stmmac/enh_desc.c:381:30: warning: symbol 'enh_desc_ops' was not declared. Should it be static?
drivers/net/ethernet/stmicro/stmmac/norm_desc.c:253:30: warning: symbol 'ndesc_ops' was not declared. Should it be static?
drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c:141:33: warning: symbol 'stmmac_ptp' was not declared. Should it be static?

There is no functional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/enh_desc.c        | 2 ++
 drivers/net/ethernet/stmicro/stmmac/norm_desc.c       | 2 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
index 1e2bcf5..ddd4272 100644
--- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
@@ -23,8 +23,10 @@
 *******************************************************************************/
 
 #include <linux/stmmac.h>
+
 #include "common.h"
 #include "descs_com.h"
+#include "stmmac.h"
 
 static int enh_desc_get_tx_status(void *data, struct stmmac_extra_stats *x,
 				  struct dma_desc *p, void __iomem *ioaddr)
diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
index 35ad4f4..46b882c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
@@ -23,8 +23,10 @@
 *******************************************************************************/
 
 #include <linux/stmmac.h>
+
 #include "common.h"
 #include "descs_com.h"
+#include "stmmac.h"
 
 static int ndesc_get_tx_status(void *data, struct stmmac_extra_stats *x,
 			       struct dma_desc *p, void __iomem *ioaddr)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
index 76ad214..88e0da3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
@@ -25,8 +25,10 @@
 
 #include <linux/io.h>
 #include <linux/delay.h>
+
 #include "common.h"
 #include "stmmac_ptp.h"
+#include "stmmac.h"
 
 static void stmmac_config_hw_tstamping(void __iomem *ioaddr, u32 data)
 {
-- 
2.1.1

[PATCH] stmmac: remove custom implementation of print_hex_dump()

[Andy Shevchenko]

There is a kernel helper to dump buffers in a hexdecimal format. This patch
substitutes the open coded function by calling that helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 6f77a46..5fb87f5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -191,14 +191,8 @@ static void stmmac_clk_csr_set(struct stmmac_priv *priv)
 
 static void print_pkt(unsigned char *buf, int len)
 {
-	int j;
-	pr_debug("len = %d byte, buf addr: 0x%p", len, buf);
-	for (j = 0; j < len; j++) {
-		if ((j % 16) == 0)
-			pr_debug("\n %03x:", j);
-		pr_debug(" %02x", buf[j]);
-	}
-	pr_debug("\n");
+	pr_debug("len = %d byte, buf addr: 0x%p\n", len, buf);
+	print_hex_dump(KERN_DEBUG, " ", DUMP_PREFIX_OFFSET, 16, 1, buf, len, 0);
 }
 
 /* minimum number of free TX descriptors required to wake up TX process */
-- 
2.1.1

Re: [PATCH] stmmac: remove custom implementation of print_hex_dump()

[Joe Perches]

On Mon, 2014-11-03 at 19:28 +0200, Andy Shevchenko wrote:
> There is a kernel helper to dump buffers in a hexdecimal format. This patch
> substitutes the open coded function by calling that helper.
[]
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
[]
> @@ -191,14 +191,8 @@ static void stmmac_clk_csr_set(struct stmmac_priv *priv)
>  
>  static void print_pkt(unsigned char *buf, int len)
>  {
> -	int j;
> -	pr_debug("len = %d byte, buf addr: 0x%p", len, buf);
> -	for (j = 0; j < len; j++) {
> -		if ((j % 16) == 0)
> -			pr_debug("\n %03x:", j);
> -		pr_debug(" %02x", buf[j]);
> -	}
> -	pr_debug("\n");
> +	pr_debug("len = %d byte, buf addr: 0x%p\n", len, buf);
> +	print_hex_dump(KERN_DEBUG, " ", DUMP_PREFIX_OFFSET, 16, 1, buf, len, 0);

Prefix should be ""
Please use true/false for the last argument

Another (better?) option would be to use:

	print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, buf, len);

so that it can be dynamically controlled like
the pr_debug statements.

Re: [PATCH] stmmac: remove custom implementation of print_hex_dump()

[Andy Shevchenko]

On Mon, 2014-11-03 at 09:39 -0800, Joe Perches wrote:
> On Mon, 2014-11-03 at 19:28 +0200, Andy Shevchenko wrote:
> > There is a kernel helper to dump buffers in a hexdecimal format. This patch
> > substitutes the open coded function by calling that helper.
> []
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> []
> > @@ -191,14 +191,8 @@ static void stmmac_clk_csr_set(struct stmmac_priv *priv)
> >  
> >  static void print_pkt(unsigned char *buf, int len)
> >  {
> > -	int j;
> > -	pr_debug("len = %d byte, buf addr: 0x%p", len, buf);
> > -	for (j = 0; j < len; j++) {
> > -		if ((j % 16) == 0)
> > -			pr_debug("\n %03x:", j);
> > -		pr_debug(" %02x", buf[j]);
> > -	}
> > -	pr_debug("\n");
> > +	pr_debug("len = %d byte, buf addr: 0x%p\n", len, buf);
> > +	print_hex_dump(KERN_DEBUG, " ", DUMP_PREFIX_OFFSET, 16, 1, buf, len, 0);
> 
> Prefix should be ""

Oh, initially there is an indentation in one space.
Maybe Giuseppe would comment on this.

> Please use true/false for the last argument

Okay.

> 
> Another (better?) option would be to use:
> 
> 	print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, buf, len);

> so that it can be dynamically controlled like
> the pr_debug statements.

Ah, yes, but unfortunately it will print ASCII part always. Giuseppe,
what do you think?

P.S. [off topic] Joe, just would like to know if you have you seen my
last version of the patch series against hexdump.c which adds
seq_hex_dump() call [1]. If so, could you comment on it?
[1] https://lkml.org/lkml/2014/9/4/309

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH] stmmac: remove custom implementation of print_hex_dump()

[Joe Perches]

On Mon, 2014-11-03 at 19:53 +0200, Andy Shevchenko wrote:
> On Mon, 2014-11-03 at 09:39 -0800, Joe Perches wrote:
> > On Mon, 2014-11-03 at 19:28 +0200, Andy Shevchenko wrote:
> > > There is a kernel helper to dump buffers in a hexdecimal format. This patch
> > > substitutes the open coded function by calling that helper.
> > []
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > []
> > > @@ -191,14 +191,8 @@ static void stmmac_clk_csr_set(struct stmmac_priv *priv)
> > >  
> > >  static void print_pkt(unsigned char *buf, int len)
> > >  {
> > > -	int j;
> > > -	pr_debug("len = %d byte, buf addr: 0x%p", len, buf);
> > > -	for (j = 0; j < len; j++) {
> > > -		if ((j % 16) == 0)
> > > -			pr_debug("\n %03x:", j);
> > > -		pr_debug(" %02x", buf[j]);
> > > -	}
> > > -	pr_debug("\n");
> > > +	pr_debug("len = %d byte, buf addr: 0x%p\n", len, buf);
> > > +	print_hex_dump(KERN_DEBUG, " ", DUMP_PREFIX_OFFSET, 16, 1, buf, len, 0);
> > 
> > Prefix should be ""
> 
> Oh, initially there is an indentation in one space.
> Maybe Giuseppe would comment on this.

I think that's not particularly important.

This changes the output anyway as the the printed offset
is now 8 chars wide not 3.

> > Another (better?) option would be to use:
> > 
> > 	print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, buf, len);
> 
> > so that it can be dynamically controlled like
> > the pr_debug statements.
> 
> Ah, yes, but unfortunately it will print ASCII part always. Giuseppe,
> what do you think?

I'm not bothered by logging message output changes.
It's not a guaranteed stable output.

> P.S. [off topic] Joe, just would like to know if you have you seen my
> last version of the patch series against hexdump.c which adds
> seq_hex_dump() call [1]. If so, could you comment on it?
> [1] https://lkml.org/lkml/2014/9/4/309

seq_<foo> output however is guaranteed.

So any conversion to the seq_hex_dump function
would need to be exactly the same.

New code could use seq_hex_dump.

Other than that, it seems sensible enough.

Re: [PATCH] stmmac: fix sparse warnings

[Giuseppe CAVALLARO]

On 11/3/2014 6:28 PM, Andy Shevchenko wrote:
> This patch fixes the following sparse warnings.
>
> drivers/net/ethernet/stmicro/stmmac/enh_desc.c:381:30: warning: symbol 'enh_desc_ops' was not declared. Should it be static?
> drivers/net/ethernet/stmicro/stmmac/norm_desc.c:253:30: warning: symbol 'ndesc_ops' was not declared. Should it be static?
> drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c:141:33: warning: symbol 'stmmac_ptp' was not declared. Should it be static?
>
> There is no functional change.

Hello Andy

I have never seen this kind of warnings. I prefer to not include the
stmmac.h in enh_desc.c and norm_desc.c but eventually to move the
following from stmmac.h to common.h:
   extern const struct stmmac_desc_ops enh_desc_ops;
   extern const struct stmmac_desc_ops ndesc_ops;
what do you think?

peppe


>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/net/ethernet/stmicro/stmmac/enh_desc.c        | 2 ++
>   drivers/net/ethernet/stmicro/stmmac/norm_desc.c       | 2 ++
>   drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c | 2 ++
>   3 files changed, 6 insertions(+)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> index 1e2bcf5..ddd4272 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> @@ -23,8 +23,10 @@
>   *******************************************************************************/
>
>   #include <linux/stmmac.h>
> +
>   #include "common.h"
>   #include "descs_com.h"
> +#include "stmmac.h"
>
>   static int enh_desc_get_tx_status(void *data, struct stmmac_extra_stats *x,
>   				  struct dma_desc *p, void __iomem *ioaddr)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> index 35ad4f4..46b882c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> @@ -23,8 +23,10 @@
>   *******************************************************************************/
>
>   #include <linux/stmmac.h>
> +
>   #include "common.h"
>   #include "descs_com.h"
> +#include "stmmac.h"
>
>   static int ndesc_get_tx_status(void *data, struct stmmac_extra_stats *x,
>   			       struct dma_desc *p, void __iomem *ioaddr)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
> index 76ad214..88e0da3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
> @@ -25,8 +25,10 @@
>
>   #include <linux/io.h>
>   #include <linux/delay.h>
> +
>   #include "common.h"
>   #include "stmmac_ptp.h"
> +#include "stmmac.h"
>
>   static void stmmac_config_hw_tstamping(void __iomem *ioaddr, u32 data)
>   {
>

Re: [PATCH] stmmac: fix sparse warnings

[David Miller]

From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
Date: Tue, 4 Nov 2014 17:35:17 +0100

> On 11/3/2014 6:28 PM, Andy Shevchenko wrote:
>> This patch fixes the following sparse warnings.
>>
>> drivers/net/ethernet/stmicro/stmmac/enh_desc.c:381:30: warning: symbol
>> 'enh_desc_ops' was not declared. Should it be static?
>> drivers/net/ethernet/stmicro/stmmac/norm_desc.c:253:30: warning:
>> symbol 'ndesc_ops' was not declared. Should it be static?
>> drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c:141:33: warning:
>> symbol 'stmmac_ptp' was not declared. Should it be static?
>>
>> There is no functional change.
> 
> Hello Andy
> 
> I have never seen this kind of warnings.

Are you running the 'sparse' checker with all options enabled?
This is one of the most fundamental warnings it spits out.

> I prefer to not include the
> stmmac.h in enh_desc.c and norm_desc.c but eventually to move the
> following from stmmac.h to common.h:
>   extern const struct stmmac_desc_ops enh_desc_ops;
>   extern const struct stmmac_desc_ops ndesc_ops;
> what do you think?

You two sort this out and submit a new patch, thanks.

Re: [PATCH] stmmac: fix sparse warnings

[Andy Shevchenko]

On Tue, 2014-11-04 at 17:35 +0100, Giuseppe CAVALLARO wrote:
> On 11/3/2014 6:28 PM, Andy Shevchenko wrote:
> > This patch fixes the following sparse warnings.
> >
> > drivers/net/ethernet/stmicro/stmmac/enh_desc.c:381:30: warning: symbol 'enh_desc_ops' was not declared. Should it be static?
> > drivers/net/ethernet/stmicro/stmmac/norm_desc.c:253:30: warning: symbol 'ndesc_ops' was not declared. Should it be static?
> > drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c:141:33: warning: symbol 'stmmac_ptp' was not declared. Should it be static?
> >
> > There is no functional change.
> 
> Hello Andy
> 
> I have never seen this kind of warnings. I prefer to not include the
> stmmac.h in enh_desc.c and norm_desc.c but eventually to move the
> following from stmmac.h to common.h:
>    extern const struct stmmac_desc_ops enh_desc_ops;
>    extern const struct stmmac_desc_ops ndesc_ops;
> what do you think?

If it would work I do this way certainly. Thanks for the tip.

Will check soon and resubmit new version.

> 
> peppe
> 
> 
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >   drivers/net/ethernet/stmicro/stmmac/enh_desc.c        | 2 ++
> >   drivers/net/ethernet/stmicro/stmmac/norm_desc.c       | 2 ++
> >   drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c | 2 ++
> >   3 files changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> > index 1e2bcf5..ddd4272 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> > @@ -23,8 +23,10 @@
> >   *******************************************************************************/
> >
> >   #include <linux/stmmac.h>
> > +
> >   #include "common.h"
> >   #include "descs_com.h"
> > +#include "stmmac.h"
> >
> >   static int enh_desc_get_tx_status(void *data, struct stmmac_extra_stats *x,
> >   				  struct dma_desc *p, void __iomem *ioaddr)
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> > index 35ad4f4..46b882c 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
> > @@ -23,8 +23,10 @@
> >   *******************************************************************************/
> >
> >   #include <linux/stmmac.h>
> > +
> >   #include "common.h"
> >   #include "descs_com.h"
> > +#include "stmmac.h"
> >
> >   static int ndesc_get_tx_status(void *data, struct stmmac_extra_stats *x,
> >   			       struct dma_desc *p, void __iomem *ioaddr)
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
> > index 76ad214..88e0da3 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
> > @@ -25,8 +25,10 @@
> >
> >   #include <linux/io.h>
> >   #include <linux/delay.h>
> > +
> >   #include "common.h"
> >   #include "stmmac_ptp.h"
> > +#include "stmmac.h"
> >
> >   static void stmmac_config_hw_tstamping(void __iomem *ioaddr, u32 data)
> >   {
> >
> 


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH] stmmac: fix sparse warnings

[Giuseppe CAVALLARO]

On 11/5/2014 10:03 AM, Andy Shevchenko wrote:
> On Tue, 2014-11-04 at 17:35 +0100, Giuseppe CAVALLARO wrote:
>> On 11/3/2014 6:28 PM, Andy Shevchenko wrote:
>>> This patch fixes the following sparse warnings.
>>>
>>> drivers/net/ethernet/stmicro/stmmac/enh_desc.c:381:30: warning: symbol 'enh_desc_ops' was not declared. Should it be static?
>>> drivers/net/ethernet/stmicro/stmmac/norm_desc.c:253:30: warning: symbol 'ndesc_ops' was not declared. Should it be static?
>>> drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c:141:33: warning: symbol 'stmmac_ptp' was not declared. Should it be static?
>>>
>>> There is no functional change.
>>
>> Hello Andy
>>
>> I have never seen this kind of warnings. I prefer to not include the
>> stmmac.h in enh_desc.c and norm_desc.c but eventually to move the
>> following from stmmac.h to common.h:
>>     extern const struct stmmac_desc_ops enh_desc_ops;
>>     extern const struct stmmac_desc_ops ndesc_ops;
>> what do you think?
>
> If it would work I do this way certainly. Thanks for the tip.

yes Andy just verified with sparse, pls also fix:

extern const struct stmmac_hwtimestamp stmmac_ptp;

in the same way.

>
> Will check soon and resubmit new version.

sure and you can add my Acked-by:

peppe

>
>>
>> peppe
>>
>>
>>>
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> ---
>>>    drivers/net/ethernet/stmicro/stmmac/enh_desc.c        | 2 ++
>>>    drivers/net/ethernet/stmicro/stmmac/norm_desc.c       | 2 ++
>>>    drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c | 2 ++
>>>    3 files changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
>>> index 1e2bcf5..ddd4272 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
>>> @@ -23,8 +23,10 @@
>>>    *******************************************************************************/
>>>
>>>    #include <linux/stmmac.h>
>>> +
>>>    #include "common.h"
>>>    #include "descs_com.h"
>>> +#include "stmmac.h"
>>>
>>>    static int enh_desc_get_tx_status(void *data, struct stmmac_extra_stats *x,
>>>    				  struct dma_desc *p, void __iomem *ioaddr)
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
>>> index 35ad4f4..46b882c 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
>>> @@ -23,8 +23,10 @@
>>>    *******************************************************************************/
>>>
>>>    #include <linux/stmmac.h>
>>> +
>>>    #include "common.h"
>>>    #include "descs_com.h"
>>> +#include "stmmac.h"
>>>
>>>    static int ndesc_get_tx_status(void *data, struct stmmac_extra_stats *x,
>>>    			       struct dma_desc *p, void __iomem *ioaddr)
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
>>> index 76ad214..88e0da3 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwtstamp.c
>>> @@ -25,8 +25,10 @@
>>>
>>>    #include <linux/io.h>
>>>    #include <linux/delay.h>
>>> +
>>>    #include "common.h"
>>>    #include "stmmac_ptp.h"
>>> +#include "stmmac.h"
>>>
>>>    static void stmmac_config_hw_tstamping(void __iomem *ioaddr, u32 data)
>>>    {
>>>
>>
>
>