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

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:[email protected]]
Sent: Wednesday, August 21, 2013 2:10 PM
To: Gole, Anant
Cc: [email protected]
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

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

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

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

Ah. Place holder code. Gotcha. Thank you for the explanation.

regards,
dan carpenter