* re: net: Add TI DaVinci EMAC driver
@ 2013-08-21 8:40 Dan Carpenter
2013-08-21 8:44 ` Gole, Anant
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Dan Carpenter @ 2013-08-21 8:40 UTC (permalink / raw)
To: kernel-janitors
Hello Anant Gole,
The patch a6286ee630f6: "net: Add TI DaVinci EMAC driver" from May
18, 2009, has the following potentially buggy code:
drivers/net/ethernet/ti/davinci_emac.c
1316 ((EMAC_DEF_ERROR_FRAME_EN) ? (EMAC_RXMBP_CEFEN_MASK) : 0x0) |
1317 ((EMAC_DEF_PROM_EN) ? (EMAC_RXMBP_CAFEN_MASK) : 0x0) |
1318 ((EMAC_DEF_PROM_CH & EMAC_RXMBP_CHMASK) << \
^^^^^^^^^^^^^^^^
This is bit 0 but it's used as a mask. It should maybe be:
EMAC_MBP_PROMISCCH(EMAC_DEF_PROM_CH) & EMAC_RXMBP_CHMASK
1319 EMAC_RXMBP_PROMCH_SHIFT) |
1320 ((EMAC_DEF_BCAST_EN) ? (EMAC_RXMBP_BROADEN_MASK) : 0x0) |
1321 ((EMAC_DEF_BCAST_CH & EMAC_RXMBP_CHMASK) << \
^^^^^^^^^^^^^^^^^
Same thing here.
1322 EMAC_RXMBP_BROADCH_SHIFT) |
1323 ((EMAC_DEF_MCAST_EN) ? (EMAC_RXMBP_MULTIEN_MASK) : 0x0) |
1324 ((EMAC_DEF_MCAST_CH & EMAC_RXMBP_CHMASK) << \
^^^^^^^^^^^^^^^^^
1325 EMAC_RXMBP_MULTICH_SHIFT));
Really, this whole section of bit masks is very hard to look at or
review.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: net: Add TI DaVinci EMAC driver
2013-08-21 8:40 net: Add TI DaVinci EMAC driver Dan Carpenter
@ 2013-08-21 8:44 ` Gole, Anant
2013-08-21 9:11 ` Mugunthan V N
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Gole, Anant @ 2013-08-21 8:44 UTC (permalink / raw)
To: kernel-janitors
Dan,
Sorry for top posting. I am looping in Mugunthan who is currently looking into this driver.
Mugunthan,
Can you look at the issue pointed out by Dan and submit a patch if this is a bug and clean up if needed?
Regards,
Anant
-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
Sent: Wednesday, August 21, 2013 2:10 PM
To: Gole, Anant
Cc: kernel-janitors@vger.kernel.org
Subject: re: net: Add TI DaVinci EMAC driver
Hello Anant Gole,
The patch a6286ee630f6: "net: Add TI DaVinci EMAC driver" from May 18, 2009, has the following potentially buggy code:
drivers/net/ethernet/ti/davinci_emac.c
1316 ((EMAC_DEF_ERROR_FRAME_EN) ? (EMAC_RXMBP_CEFEN_MASK) : 0x0) |
1317 ((EMAC_DEF_PROM_EN) ? (EMAC_RXMBP_CAFEN_MASK) : 0x0) |
1318 ((EMAC_DEF_PROM_CH & EMAC_RXMBP_CHMASK) << \
^^^^^^^^^^^^^^^^ This is bit 0 but it's used as a mask. It should maybe be:
EMAC_MBP_PROMISCCH(EMAC_DEF_PROM_CH) & EMAC_RXMBP_CHMASK
1319 EMAC_RXMBP_PROMCH_SHIFT) |
1320 ((EMAC_DEF_BCAST_EN) ? (EMAC_RXMBP_BROADEN_MASK) : 0x0) |
1321 ((EMAC_DEF_BCAST_CH & EMAC_RXMBP_CHMASK) << \
^^^^^^^^^^^^^^^^^ Same thing here.
1322 EMAC_RXMBP_BROADCH_SHIFT) |
1323 ((EMAC_DEF_MCAST_EN) ? (EMAC_RXMBP_MULTIEN_MASK) : 0x0) |
1324 ((EMAC_DEF_MCAST_CH & EMAC_RXMBP_CHMASK) << \
^^^^^^^^^^^^^^^^^
1325 EMAC_RXMBP_MULTICH_SHIFT));
Really, this whole section of bit masks is very hard to look at or review.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: net: Add TI DaVinci EMAC driver
2013-08-21 8:40 net: Add TI DaVinci EMAC driver Dan Carpenter
2013-08-21 8:44 ` Gole, Anant
@ 2013-08-21 9:11 ` Mugunthan V N
2013-08-21 9:26 ` Dan Carpenter
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Mugunthan V N @ 2013-08-21 9:11 UTC (permalink / raw)
To: kernel-janitors
On Wednesday 21 August 2013 02:14 PM, Gole, Anant wrote:
> Dan,
> Sorry for top posting. I am looping in Mugunthan who is currently looking into this driver.
>
> Mugunthan,
> Can you look at the issue pointed out by Dan and submit a patch if this is a bug and clean up if needed?
>
>
> Regards,
> Anant
>
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Wednesday, August 21, 2013 2:10 PM
> To: Gole, Anant
> Cc: kernel-janitors@vger.kernel.org
> Subject: re: net: Add TI DaVinci EMAC driver
>
> Hello Anant Gole,
>
> The patch a6286ee630f6: "net: Add TI DaVinci EMAC driver" from May 18, 2009, has the following potentially buggy code:
>
> drivers/net/ethernet/ti/davinci_emac.c
> 1316 ((EMAC_DEF_ERROR_FRAME_EN) ? (EMAC_RXMBP_CEFEN_MASK) : 0x0) |
> 1317 ((EMAC_DEF_PROM_EN) ? (EMAC_RXMBP_CAFEN_MASK) : 0x0) |
> 1318 ((EMAC_DEF_PROM_CH & EMAC_RXMBP_CHMASK) << \
> ^^^^^^^^^^^^^^^^ This is bit 0 but it's used as a mask. It should maybe be:
>
> EMAC_MBP_PROMISCCH(EMAC_DEF_PROM_CH) & EMAC_RXMBP_CHMASK
EMAC_DEF_PROM_CH is not denoting bit 0 but denoting which DMA channel to
be used to transfer promiscuous packet (i.e other MAC packets which
doesn't belong to us) from EMAC to DDR.
The existing code is correct but it can be simplified as you have
mentioned here to make code review simpler. Will prepare a patch and
submit to mailing list soon.
Regards
Mugunthan V N
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: net: Add TI DaVinci EMAC driver
2013-08-21 8:40 net: Add TI DaVinci EMAC driver Dan Carpenter
2013-08-21 8:44 ` Gole, Anant
2013-08-21 9:11 ` Mugunthan V N
@ 2013-08-21 9:26 ` Dan Carpenter
2013-08-21 9:33 ` Mugunthan V N
2013-08-21 10:07 ` Dan Carpenter
4 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2013-08-21 9:26 UTC (permalink / raw)
To: kernel-janitors
On Wed, Aug 21, 2013 at 08:11:10PM +0530, Mugunthan V N wrote:
> On Wednesday 21 August 2013 02:14 PM, Gole, Anant wrote:
> > drivers/net/ethernet/ti/davinci_emac.c
> > 1316 ((EMAC_DEF_ERROR_FRAME_EN) ? (EMAC_RXMBP_CEFEN_MASK) : 0x0) |
> > 1317 ((EMAC_DEF_PROM_EN) ? (EMAC_RXMBP_CAFEN_MASK) : 0x0) |
> > 1318 ((EMAC_DEF_PROM_CH & EMAC_RXMBP_CHMASK) << \
> > ^^^^^^^^^^^^^^^^ This is bit 0 but it's used as a mask. It should maybe be:
> >
> > EMAC_MBP_PROMISCCH(EMAC_DEF_PROM_CH) & EMAC_RXMBP_CHMASK
> EMAC_DEF_PROM_CH is not denoting bit 0 but denoting which DMA channel to
> be used to transfer promiscuous packet (i.e other MAC packets which
> doesn't belong to us) from EMAC to DDR.
>
> The existing code is correct but it can be simplified as you have
> mentioned here to make code review simpler. Will prepare a patch and
> submit to mailing list soon.
Let me say this a different way "(0 & x) is always zero". We can
just delete it because we know:
((EMAC_DEF_PROM_CH & EMAC_RXMBP_CHMASK) << \
EMAC_RXMBP_PROMCH_SHIFT)
We know that 0 ANDed and then shifted is still zero.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: net: Add TI DaVinci EMAC driver
2013-08-21 8:40 net: Add TI DaVinci EMAC driver Dan Carpenter
` (2 preceding siblings ...)
2013-08-21 9:26 ` Dan Carpenter
@ 2013-08-21 9:33 ` Mugunthan V N
2013-08-21 10:07 ` Dan Carpenter
4 siblings, 0 replies; 6+ messages in thread
From: Mugunthan V N @ 2013-08-21 9:33 UTC (permalink / raw)
To: kernel-janitors
On Wednesday 21 August 2013 02:56 PM, Dan Carpenter wrote:
> On Wed, Aug 21, 2013 at 08:11:10PM +0530, Mugunthan V N wrote:
>> On Wednesday 21 August 2013 02:14 PM, Gole, Anant wrote:
>>> drivers/net/ethernet/ti/davinci_emac.c
>>> 1316 ((EMAC_DEF_ERROR_FRAME_EN) ? (EMAC_RXMBP_CEFEN_MASK) : 0x0) |
>>> 1317 ((EMAC_DEF_PROM_EN) ? (EMAC_RXMBP_CAFEN_MASK) : 0x0) |
>>> 1318 ((EMAC_DEF_PROM_CH & EMAC_RXMBP_CHMASK) << \
>>> ^^^^^^^^^^^^^^^^ This is bit 0 but it's used as a mask. It should maybe be:
>>>
>>> EMAC_MBP_PROMISCCH(EMAC_DEF_PROM_CH) & EMAC_RXMBP_CHMASK
>> EMAC_DEF_PROM_CH is not denoting bit 0 but denoting which DMA channel to
>> be used to transfer promiscuous packet (i.e other MAC packets which
>> doesn't belong to us) from EMAC to DDR.
>>
>> The existing code is correct but it can be simplified as you have
>> mentioned here to make code review simpler. Will prepare a patch and
>> submit to mailing list soon.
> Let me say this a different way "(0 & x) is always zero". We can
> just delete it because we know:
>
> ((EMAC_DEF_PROM_CH & EMAC_RXMBP_CHMASK) << \
> EMAC_RXMBP_PROMCH_SHIFT)
>
> We know that 0 ANDed and then shifted is still zero.
>
>
Lets take channel 1 (i.e. channel 1 for promiscuous packet) as example
which will make it more clear. 1 & 7 will be 1, & 7 is to make a upper
limit and not to choose any channel greater than 7 and now 1 << 16 makes
sense. Since in current driver we choose channel 0 for promiscuous
packet so it looks like it makes no sense.
So when some one wants to use channel 1 to 7, then the above code will
make a difference in DMA performance.
Regards
Mugunthan V N
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: net: Add TI DaVinci EMAC driver
2013-08-21 8:40 net: Add TI DaVinci EMAC driver Dan Carpenter
` (3 preceding siblings ...)
2013-08-21 9:33 ` Mugunthan V N
@ 2013-08-21 10:07 ` Dan Carpenter
4 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2013-08-21 10:07 UTC (permalink / raw)
To: kernel-janitors
Ah. Place holder code. Gotcha. Thank you for the explanation.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-08-21 10:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-21 8:40 net: Add TI DaVinci EMAC driver Dan Carpenter
2013-08-21 8:44 ` Gole, Anant
2013-08-21 9:11 ` Mugunthan V N
2013-08-21 9:26 ` Dan Carpenter
2013-08-21 9:33 ` Mugunthan V N
2013-08-21 10:07 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox