On 05/15/2010 07:26 PM, Oliver Hartkopp wrote:
> On 14.05.2010 15:44, Marc Kleine-Budde wrote:
>> Oliver Hartkopp wrote:
>
>>>
>>> It took a while to create a minimum invasive patch that doesn't touch all
>>> the
>>> drivers, but here it is:
>>>
>>> Index: sja1000/sja1000.c
>>> ===================================================================
>>> --- sja1000/sja1000.c (Revision 1177)
>>> +++ sja1000/sja1000.c (Arbeitskopie)
>>> @@ -88,6 +88,29 @@
>>> .brp_inc = 1,
>>> };
>>>
>>> +
>>> +static void sja1000_write_cmd_reg(struct sja1000_priv *priv, u8 val)
>>> +{
>>> + /* the command register needs some locking in SMP systems */
>>
>> nitpick:
>> from my school english point of view: on SMP systems sounds better
>>
>
> Indeed :-)
>
>>> +
>>> +#ifdef CONFIG_SMP
>>> +
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&priv->wr_reg_lock, flags);
>>> + priv->write_reg(priv, REG_CMR, val);
>>> + priv->read_reg(priv, REG_SR);
>>> + spin_unlock_irqrestore(&priv->wr_reg_lock, flags);
>>> +
>>> +#else
>>> +
>>> + /* write to the command register without locking */
>>> + priv->write_reg(priv, REG_CMR, val);
>>> +
>>> +#endif
>>> +}
>>> +
>>> +
>>> static int sja1000_probe_chip(struct net_device *dev)
>>> {
>>> struct sja1000_priv *priv = netdev_priv(dev);
>>> @@ -305,7 +328,7 @@
>>>
>>> can_put_echo_skb(skb, dev, 0);
>>>
>>> - priv->write_reg(priv, REG_CMR, CMD_TR);
>>> + sja1000_write_cmd_reg(priv, CMD_TR);
>>>
>>> return NETDEV_TX_OK;
>>> }
>>> @@ -358,7 +381,7 @@
>>> cf->can_id = id;
>>>
>>> /* release receive buffer */
>>> - priv->write_reg(priv, REG_CMR, CMD_RRB);
>>> + sja1000_write_cmd_reg(priv, CMD_RRB);
>>>
>>> netif_rx(skb);
>>>
>>> @@ -393,7 +416,7 @@
>>> cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
>>> stats->rx_over_errors++;
>>> stats->rx_errors++;
>>> - priv->write_reg(priv, REG_CMR, CMD_CDO); /* clear bit */
>>> + sja1000_write_cmd_reg(priv, CMD_CDO); /* clear bit */
>>> }
>>>
>>> if (isrc & IRQ_EI) {
>>> Index: sja1000/sja1000.h
>>> ===================================================================
>>> --- sja1000/sja1000.h (Revision 1177)
>>> +++ sja1000/sja1000.h (Arbeitskopie)
>>> @@ -168,6 +168,7 @@
>>> void __iomem *reg_base; /* ioremap'ed address to registers */
>>> unsigned long irq_flags; /* for request_irq() */
>>>
>>> + spinlock_t wr_reg_lock; /* lock for concurrent register writes */
>>
>> hmm...do we want to wrap this is ifdef CONFIG_SMP?
>
> Would be ok for me.
>
>>
>>> u16 flags; /* custom mode flags */
>>> u8 ocr; /* output control register */
>>> u8 cdr; /* clock divider register */
>>>
>>>
>>> Any Acked-by or Signed-off-by out there?
>
> Any other feedback?
You can add my "Acked-by".
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
