* re: amd-xgbe: Initial AMD 10GbE platform driver
@ 2014-11-12 12:03 Dan Carpenter
2014-11-12 12:08 ` Dan Carpenter
2014-11-12 16:57 ` Tom Lendacky
0 siblings, 2 replies; 3+ messages in thread
From: Dan Carpenter @ 2014-11-12 12:03 UTC (permalink / raw)
To: kernel-janitors
Hello Lendacky, Thomas,
The patch c5aa9e3b8156: "amd-xgbe: Initial AMD 10GbE platform driver"
from Jun 5, 2014, leads to the following Smatch warning:
drivers/net/ethernet/amd/xgbe/xgbe-dev.c:1633 xgbe_dev_read()
warn: we tested 'err' before and it was 'true'
drivers/net/ethernet/amd/xgbe/xgbe-dev.c
1628 /* Check for errors (only valid in last descriptor) */
1629 err = XGMAC_GET_BITS_LE(rdesc->desc3, RX_NORMAL_DESC3, ES);
1630 etlt = XGMAC_GET_BITS_LE(rdesc->desc3, RX_NORMAL_DESC3, ETLT);
1631 DBGPR(" err=%u, etlt=%#x\n", err, etlt);
1632
1633 if (!err || (err && !etlt)) {
^^^^^^^^^^^^^^^^^^^^^^
Smatch is saying that we could re-write this as:
if (!err || !etlt) {
Which is true, but I'm not sure if that was really the original intent
of the code. If it was then maybe we could make it more explicit by
saying:
if (etlt = 0x00) {
/* do nothing */
} else if (!err) {
if (etlt = 0x09 &&
...
} else {
if (etlt = 0x05 || etlt = 0x06)
...
1634 if ((etlt = 0x09) &&
1635 (netdev->features & NETIF_F_HW_VLAN_CTAG_RX)) {
1636 XGMAC_SET_BITS(packet->attributes, RX_PACKET_ATTRIBUTES,
1637 VLAN_CTAG, 1);
1638 packet->vlan_ctag = XGMAC_GET_BITS_LE(rdesc->desc0,
1639 RX_NORMAL_DESC0,
1640 OVT);
1641 DBGPR(" vlan-ctag=0x%04x\n", packet->vlan_ctag);
1642 }
1643 } else {
1644 if ((etlt = 0x05) || (etlt = 0x06))
1645 XGMAC_SET_BITS(packet->attributes, RX_PACKET_ATTRIBUTES,
1646 CSUM_DONE, 0);
1647 else
1648 XGMAC_SET_BITS(packet->errors, RX_PACKET_ERRORS,
1649 FRAME, 1);
1650 }
1651
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: amd-xgbe: Initial AMD 10GbE platform driver
2014-11-12 12:03 amd-xgbe: Initial AMD 10GbE platform driver Dan Carpenter
@ 2014-11-12 12:08 ` Dan Carpenter
2014-11-12 16:57 ` Tom Lendacky
1 sibling, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2014-11-12 12:08 UTC (permalink / raw)
To: kernel-janitors
On Wed, Nov 12, 2014 at 03:03:29PM +0300, Dan Carpenter wrote:
>
> if (etlt = 0x00) {
=. I am become derp.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: amd-xgbe: Initial AMD 10GbE platform driver
2014-11-12 12:03 amd-xgbe: Initial AMD 10GbE platform driver Dan Carpenter
2014-11-12 12:08 ` Dan Carpenter
@ 2014-11-12 16:57 ` Tom Lendacky
1 sibling, 0 replies; 3+ messages in thread
From: Tom Lendacky @ 2014-11-12 16:57 UTC (permalink / raw)
To: kernel-janitors
On 11/12/2014 06:03 AM, Dan Carpenter wrote:
> Hello Lendacky, Thomas,
>
> The patch c5aa9e3b8156: "amd-xgbe: Initial AMD 10GbE platform driver"
> from Jun 5, 2014, leads to the following Smatch warning:
>
> drivers/net/ethernet/amd/xgbe/xgbe-dev.c:1633 xgbe_dev_read()
> warn: we tested 'err' before and it was 'true'
>
> drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> 1628 /* Check for errors (only valid in last descriptor) */
> 1629 err = XGMAC_GET_BITS_LE(rdesc->desc3, RX_NORMAL_DESC3, ES);
> 1630 etlt = XGMAC_GET_BITS_LE(rdesc->desc3, RX_NORMAL_DESC3, ETLT);
> 1631 DBGPR(" err=%u, etlt=%#x\n", err, etlt);
> 1632
> 1633 if (!err || (err && !etlt)) {
> ^^^^^^^^^^^^^^^^^^^^^^
> Smatch is saying that we could re-write this as:
>
> if (!err || !etlt) {
>
> Which is true, but I'm not sure if that was really the original intent
> of the code. If it was then maybe we could make it more explicit by
The original intent is to handle the case where err is zero (no error)
or err is one but etlt is zero (no error, even though err is one).
I'll step back a bit and try to see what looks best. I'm leaning
towards the Smatch suggestion and I'll just add a comment explaining
it.
Thanks,
Tom
> saying:
>
> if (etlt = 0x00) {
> /* do nothing */
> } else if (!err) {
> if (etlt = 0x09 &&
> ...
> } else {
> if (etlt = 0x05 || etlt = 0x06)
> ...
>
> 1634 if ((etlt = 0x09) &&
> 1635 (netdev->features & NETIF_F_HW_VLAN_CTAG_RX)) {
> 1636 XGMAC_SET_BITS(packet->attributes, RX_PACKET_ATTRIBUTES,
> 1637 VLAN_CTAG, 1);
> 1638 packet->vlan_ctag = XGMAC_GET_BITS_LE(rdesc->desc0,
> 1639 RX_NORMAL_DESC0,
> 1640 OVT);
> 1641 DBGPR(" vlan-ctag=0x%04x\n", packet->vlan_ctag);
> 1642 }
> 1643 } else {
> 1644 if ((etlt = 0x05) || (etlt = 0x06))
> 1645 XGMAC_SET_BITS(packet->attributes, RX_PACKET_ATTRIBUTES,
> 1646 CSUM_DONE, 0);
> 1647 else
> 1648 XGMAC_SET_BITS(packet->errors, RX_PACKET_ERRORS,
> 1649 FRAME, 1);
> 1650 }
> 1651
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-11-12 16:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-12 12:03 amd-xgbe: Initial AMD 10GbE platform driver Dan Carpenter
2014-11-12 12:08 ` Dan Carpenter
2014-11-12 16:57 ` Tom Lendacky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).