[bug report] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

[bug report] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

[Dan Carpenter]

Hello Łukasz Stelmach,

The patch a97c69ba4f30: "net: ax88796c: ASIX AX88796C SPI Ethernet
Adapter Driver" from Oct 20, 2021, leads to the following Smatch
static checker warning:

	drivers/net/ethernet/asix/ax88796c_main.c:391 ax88796c_start_xmit()
	warn: test_bit() takes a bit number

drivers/net/ethernet/asix/ax88796c_main.c
    382 static int
    383 ax88796c_start_xmit(struct sk_buff *skb, struct net_device *ndev)
    384 {
    385         struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
    386 
    387         skb_queue_tail(&ax_local->tx_wait_q, skb);
    388         if (skb_queue_len(&ax_local->tx_wait_q) > TX_QUEUE_HIGH_WATER)
    389                 netif_stop_queue(ndev);
    390 
--> 391         set_bit(EVENT_TX, &ax_local->flags);

EVENT_TX is BIT(1) so this ends up being a double BIT(BIT(1));.  Which
is fine because it seems to be done consistently.  But probably it
should be:

#define EVENT_INTR		0
#define EVENT_TX		1
#define EVENT_SET_MULTI		2

    392         schedule_work(&ax_local->ax_work);
    393 
    394         return NETDEV_TX_OK;
    395 }

regards,
dan carpenter

Re: [bug report] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

[Lukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 1629 bytes --]

It was <2021-11-12 pią 15:36>, when Dan Carpenter wrote:
> Hello Łukasz Stelmach,
>
> The patch a97c69ba4f30: "net: ax88796c: ASIX AX88796C SPI Ethernet
> Adapter Driver" from Oct 20, 2021, leads to the following Smatch
> static checker warning:
>
> 	drivers/net/ethernet/asix/ax88796c_main.c:391 ax88796c_start_xmit()
> 	warn: test_bit() takes a bit number
>
> drivers/net/ethernet/asix/ax88796c_main.c
>     382 static int
>     383 ax88796c_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>     384 {
>     385         struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>     386 
>     387         skb_queue_tail(&ax_local->tx_wait_q, skb);
>     388         if (skb_queue_len(&ax_local->tx_wait_q) > TX_QUEUE_HIGH_WATER)
>     389                 netif_stop_queue(ndev);
>     390 
> --> 391         set_bit(EVENT_TX, &ax_local->flags);
>
> EVENT_TX is BIT(1) so this ends up being a double BIT(BIT(1));.  Which
> is fine because it seems to be done consistently.  But probably it
> should be:
>
> #define EVENT_INTR		0
> #define EVENT_TX		1
> #define EVENT_SET_MULTI		2
>

Apparently this is a porting artifact. Originally these were defined as
{1,2,4}[1] so I changed that to BIT(x) without much
consideration. I am fixing it. Thanks for reporting.


>     392         schedule_work(&ax_local->ax_work);
>     393 
>     394         return NETDEV_TX_OK;
>     395 }
>
> regards,
> dan carpenter
>
>

[1] https://lore.kernel.org/lkml/20200825170311.24886-1-l.stelmach@samsung.com/
-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: [bug report] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

[Dan Carpenter]

On Tue, Nov 16, 2021 at 01:43:24AM +0100, Lukasz Stelmach wrote:
> It was <2021-11-12 pią 15:36>, when Dan Carpenter wrote:
> > Hello Łukasz Stelmach,
> >
> > The patch a97c69ba4f30: "net: ax88796c: ASIX AX88796C SPI Ethernet
> > Adapter Driver" from Oct 20, 2021, leads to the following Smatch
> > static checker warning:
> >
> > 	drivers/net/ethernet/asix/ax88796c_main.c:391 ax88796c_start_xmit()
> > 	warn: test_bit() takes a bit number
> >
> > drivers/net/ethernet/asix/ax88796c_main.c
> >     382 static int
> >     383 ax88796c_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> >     384 {
> >     385         struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
> >     386 
> >     387         skb_queue_tail(&ax_local->tx_wait_q, skb);
> >     388         if (skb_queue_len(&ax_local->tx_wait_q) > TX_QUEUE_HIGH_WATER)
> >     389                 netif_stop_queue(ndev);
> >     390 
> > --> 391         set_bit(EVENT_TX, &ax_local->flags);
> >
> > EVENT_TX is BIT(1) so this ends up being a double BIT(BIT(1));.  Which
> > is fine because it seems to be done consistently.  But probably it
> > should be:
> >
> > #define EVENT_INTR		0
> > #define EVENT_TX		1
> > #define EVENT_SET_MULTI		2
> >
> 
> Apparently this is a porting artifact. Originally these were defined as
> {1,2,4}[1] so I changed that to BIT(x) without much
> consideration. I am fixing it. Thanks for reporting.

Changing it back to 1,2,4 will hide bug from the Smatch but the correct
values are 0,1,2.

regards,
dan carpenter

Re: [bug report] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

[Lukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

It was <2021-11-11 czw 16:35>, when Dan Carpenter wrote:
> Hello Łukasz Stelmach,
>
> The patch a97c69ba4f30: "net: ax88796c: ASIX AX88796C SPI Ethernet
> Adapter Driver" from Oct 20, 2021, leads to the following Smatch
> static checker warning:
>
> 	drivers/net/ethernet/asix/ax88796c_main.c:926 ax88796c_set_features()
> 	warn: duplicate check 'changed & ((1 << (40)) | (1 << (3)))' (previous on line 921)
>
> drivers/net/ethernet/asix/ax88796c_main.c
>     915 static int
>     916 ax88796c_set_features(struct net_device *ndev, netdev_features_t features)
>     917 {
>     918         struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>     919         netdev_features_t changed = features ^ ndev->features;
>     920 
>     921         if (!(changed & (NETIF_F_RXCSUM | NETIF_F_HW_CSUM)))
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>     922                 return 0;
>     923 
>     924         ndev->features = features;
>     925 
> --> 926         if (changed & (NETIF_F_RXCSUM | NETIF_F_HW_CSUM))
>
> We know this condition is true.  Was something else intended?
>

To have a readable template to add more ifs below with other features.
In such case the if at line 921 would check for all supported
features and this and following ifs would act upon different flags.

>     927                 ax88796c_set_csums(ax_local);
>     928 
>     929         return 0;
>     930 }
>
> regards,
> dan carpenter
>
>

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

[bug report] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

[Dan Carpenter]

Hello Łukasz Stelmach,

The patch a97c69ba4f30: "net: ax88796c: ASIX AX88796C SPI Ethernet
Adapter Driver" from Oct 20, 2021, leads to the following Smatch
static checker warning:

	drivers/net/ethernet/asix/ax88796c_main.c:926 ax88796c_set_features()
	warn: duplicate check 'changed & ((1 << (40)) | (1 << (3)))' (previous on line 921)

drivers/net/ethernet/asix/ax88796c_main.c
    915 static int
    916 ax88796c_set_features(struct net_device *ndev, netdev_features_t features)
    917 {
    918         struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
    919         netdev_features_t changed = features ^ ndev->features;
    920 
    921         if (!(changed & (NETIF_F_RXCSUM | NETIF_F_HW_CSUM)))
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

    922                 return 0;
    923 
    924         ndev->features = features;
    925 
--> 926         if (changed & (NETIF_F_RXCSUM | NETIF_F_HW_CSUM))

We know this condition is true.  Was something else intended?

    927                 ax88796c_set_csums(ax_local);
    928 
    929         return 0;
    930 }

regards,
dan carpenter