netdev - Re: [PATCH] net/can: add driver for mscan family & mpc52xx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4B011001.807@grandegger.com>
Date:	Mon, 16 Nov 2009 09:40:33 +0100
From:	Wolfgang Grandegger <wg@...ndegger.com>
To:	Grant Likely <grant.likely@...retlab.ca>
CC:	Wolfram Sang <w.sang@...gutronix.de>, netdev@...r.kernel.org,
	socketcan-core@...ts.berlios.de, linuxppc-dev@...abs.org,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH] net/can: add driver for mscan family & mpc52xx_mscan

Grant Likely wrote:
> On Fri, Nov 13, 2009 at 9:14 AM, Wolfram Sang <w.sang@...gutronix.de> wrote:
>> Taken from socketcan-svn, fixed remaining todos, cleaned up, tested with a
>> phyCORE-MPC5200B-IO and a custom board.
>>
>> Signed-off-by: Wolfram Sang <w.sang@...gutronix.de>
>> Cc: Wolfgang Grandegger <wg@...ndegger.com>
>> Cc: Grant Likely <grant.likely@...retlab.ca>
>> Cc: David Miller <davem@...emloft.net>
> 
> I don't see any locking in this driver.  What keeps the mscan_isr or
> other routines from conflicting with each other?  What is the
> concurrency model for CAN devices?

There is concurrency between the mscan_start_xmit() and mscan_irq()
routine, which is handled by disabling/enabling the TX interrupt source.
CAN configuration (bit-timing) can only be changed when the device is
stopped (down) and bus-off recovery requires that interrupts are
disabled or the hadrware does not send/receive messages after the
bus-off occurred.

> More comments below.  I don't have the background to delve into the
> CAN details, but I can make some comments on the general structure of
> the driver.

[snip]
>> +static unsigned int  __devinit mpc52xx_can_clock_freq(struct of_device *of,
>> +                                                     int clock_src)
>> +{
>> +       unsigned int pvr;
>> +
>> +       pvr = mfspr(SPRN_PVR);
>> +
>> +       if (clock_src == MSCAN_CLKSRC_BUS || pvr == 0x80822011)
>> +               return mpc5xxx_get_bus_frequency(of->node);
>> +
>> +       return mpc52xx_can_xtal_freq(of);
>> +}
> 
> mpc52xx_can_xtal_freq() is only used by this function.  Do they need
> to be separate?

Not really, and it would save some lines of code.

[snip]
>> +static int mscan_set_mode(struct net_device *dev, u8 mode)
>> +{
>> +       struct mscan_priv *priv = netdev_priv(dev);
>> +       struct mscan_regs *regs = (struct mscan_regs *)priv->reg_base;
>> +       int ret = 0;
>> +       int i;
>> +       u8 canctl1;
>> +
>> +       if (mode != MSCAN_NORMAL_MODE) {
>> +
>> +               if (priv->tx_active) {
>> +                       /* Abort transfers before going to sleep */#
>> +                       out_8(&regs->cantarq, priv->tx_active);
>> +                       /* Suppress TX done interrupts */
>> +                       out_8(&regs->cantier, 0);
>> +               }
>> +
>> +               canctl1 = in_8(&regs->canctl1);
>> +               if ((mode & MSCAN_SLPRQ) && (canctl1 & MSCAN_SLPAK) == 0) {
>> +                       out_8(&regs->canctl0,
>> +                             in_8(&regs->canctl0) | MSCAN_SLPRQ);
>> +                       for (i = 0; i < MSCAN_SET_MODE_RETRIES; i++) {
>> +                               if (in_8(&regs->canctl1) & MSCAN_SLPAK)
>> +                                       break;
>> +                               udelay(100);
> 
> Ugh.  Can you sleep instead?  This burns a lot of CPU cycles to no purpose.

A real sleep for 100us? The usual jiffy based sleep would take 1..10 ms,
at least. I think we should check how much time/cycles it usually takes.

[snip]
>> +static int mscan_do_set_mode(struct net_device *dev, enum can_mode mode)
>> +{
>> +
>> +       struct mscan_priv *priv = netdev_priv(dev);
>> +       int ret = 0;
>> +
>> +       if (!priv->open_time)
>> +               return -EINVAL;
>> +
>> +       switch (mode) {
>> +       case CAN_MODE_SLEEP:
>> +       case CAN_MODE_STOP:
>> +               netif_stop_queue(dev);
>> +               mscan_set_mode(dev,
>> +                              (mode ==
>> +                               CAN_MODE_STOP) ? MSCAN_INIT_MODE :
>> +                              MSCAN_SLEEP_MODE);
> 
> A little hard on the eyes.  Can you rework to not spill over 4 lines?
> (ie. calc mode flag on the line above?)

These cases can safely be removed as currently only CAN_MODE_START is
supported by the upper layer.

Wolfgang.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html