Re: can/sja1000: potential issue in sja1000.c?

[Sebastian Haas <dev@sebastianhaas.info>]

Am 10.02.2012 12:44, schrieb Wolfgang Grandegger:
> On 02/10/2012 12:16 PM, Marc Kleine-Budde wrote:
>> On 02/10/2012 12:00 PM, Stephane Grosjean wrote:
>>> Hi all,
>>>
>>> While always fighting against pcmcia Oliver (very hot) unplug issue, I
>>> decided to have a look to the sja1000 lib, since the pcmcia driver ISR
>>> does call "sja1000_interrupt()",
>>>
>>> Unfortunately, I found that:
>>>
>>>                  if (isrc&  IRQ_RI) {
>>>                          /* receive interrupt */
>>>                          while (status&  SR_RBS) {
>>>                                  sja1000_rx(dev);
>>>                                  status = priv->read_reg(priv, REG_SR);
>>>                          }
>>>                  }
>>>
>>> My problem is, once the card is unplugged, every ioread in its
>>> corresponding io space does return 0xff...
>>> I just change this potential infinite while() into a corresponding
>>> for(;;) loop to test, like this:
>>>
>>> -                        while (status&  SR_RBS) {
>>> +                        int i;
>>> +                        for (i = 0; (status&  SR_RBS)&&  (i<  10); i++) {
>>>
>>> And no more PC hang!
>>>
>>> Can you confirm this, please?
>>>
>>> @Oliver: IMHO, the 0xff (invalid) value is also the reason of the
>>> "wakeup interrupt" message you got when unplugging the cards from their
>>> slots...
>>
>> Is 0xff a valid value for REG_SR in the first place? If not we can for
>> example exit the interrupt handler.
>
> Is the access to the address space of an unplugged card defined at all?
> Or may it even hang the system?
AFAIK PCMCIA is based on ISA and therefor it is absolutely valid to read 
and even write into this address space. It is up to user to check if the 
device is still there and accessiable.

For that ems_pcmcia does something like this:
static irqreturn_t ems_pcmcia_interrupt(int irq, void *dev_id)
{
...
   /* Card not present */
   if (readw(card->base_addr) != 0xAA55)
     return IRQ_HANDLED;

   Call sja1000_interrupt() ...
...
}

Stephané can you do something like this as well?

Cheers,
  Sebastian