Re: [PATCH]: be2iscsi: Fix MSIX interrupt names

[Rolf Eike Beer <eike-kernel@sf-tec.de>]

[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]

Am Dienstag, 24. Mai 2011, 10:48:09 schrieb Prarit Bhargava:
> On 05/20/2011 04:17 PM, Rolf Eike Beer wrote:
> > This could be simpler if you would use devres and devm_kzalloc() and
> > devm_request_irq(). You simply need to return with error then and the
> > driver core would free everything you already allocated.
> > 
> > Eike
> 
> Thanks for the suggestion Eike.
> 
> I've never used devres before.  This seems to work -- please review as [v3].

No, sorry, this wont work. You need to change your call of pci_enable_device() 
to pcim_enable_device(). Afterwards you should check what else in your probe 
routine can be converted to devres. This is optional, but why duplicate work?

What you need to take care of: resources that you do not allocate by devres 
(e.g. the scsi_host) and which are explicitely freed by you must not be needed 
e.g. in the IRQ handler if that would be freed by devres, i.e.:

int ret;
pcim_enable_device()
beiscsi_hba_alloc()
devm_request_irq()

ret = something();
if (ret != 0) {
 beiscsi_free_hba();
 return ret;
}
// devres would now free IRQs etc.

If an IRQ happens right before it is freed by devres (which could e.g. happen 
if you enable IRQ debugging) you could hit a NULL or stale host pointer. If 
your IRQ handler requires some resources to always be present (e.g. the 
scsi_host) then you must explicitely deregister it before releasing those 
resources. At the end this makes the init savings void. So you better add a 
single check

if (unlikely(my_hba == NULL))
  return IRQ_NONE;

to your IRQ handler to be safe.

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]