[ofa-general] Re: [PATCH] core/mthca: Distinguish multiple IB cards in /proc/interrupts

[Roland Dreier]

 > When the mthca driver calls request_irq() to allocate interrupt resources, it uses
 > the fixed device name string "ib_mthca". When multiple IB cards are present in the system,
 > every instance of the resource is named "ib_mthca" in /proc/interrupts.
 > This can make it very confusing trying to work out exactly where IB interrupts are going and why.

Fundamentally makes sense.  Some comments about the specifics:

 > o Added a new IB core API , ib_init_device() that allocates an ib_device struct
 >   and initializes its device name.

seems reasonable.  However I don't think we need both ib_init_device()
and ib_alloc_device(), and also the "ib_init_device" name doesn't imply
that it is allocating memory.

 > o Modified device name parameter to request_irq() to use the device name 
 >   allocated by ib_init_device()

You only did this for mthca and only in the MSI-X case.  I would suggest
that mthca at least needs to be consistent between MSI-X and non-MSI-X,
and it would be desirable to convert other drivers as well.

Also the mthca changes really should be separated out from the changes
to the core API.

So I would suggest reworking this into a series of patches:

1. Add a function ib_alloc_device_set_name() that does what your
   ib_init_device() function does.  (By the way, there is a problem with
   your implementation, since alloc_name() just checks the list of
   registered devices for a collision -- so devices that are allocated
   but not registered could be assigned the same name, if the kernel
   ever moves to parallelizing PCI probing or something like that -- so
   you should probably fix alloc_name() to check a list of all allocated
   devices or something like that)

2. For each RDMA driver (ie each of drivers/infiniband/hw/xxx), convert
   to using ib_init_device_alloc_name() -- one patch per driver.

3. Remove the old ib_alloc_device() and rename
   ib_alloc_device_set_name() back to ib_alloc_device().

4. Change mthca to use the device name when naming IRQs, both in MSI-X
   and INTx mode.

5. [optional] Have other drivers name their IRQs similarly.

One specific thing that puzzles me.  You add a field:

@@ -360,6 +360,7 @@ struct mthca_dev {
 	struct ib_ah         *sm_ah[MTHCA_MAX_PORTS];
 	spinlock_t            sm_lock;
 	u8                    rate[MTHCA_MAX_PORTS];
+	char                  irq_name[MTHCA_NUM_EQ][IB_DEVICE_NAME_MAX];
 };

which looks sane, but then the way you use it is:

 > +			strcpy(&dev->irq_name[i][IB_DEVICE_NAME_MAX], dev->ib_dev.name);
 > +			strcat(&dev->irq_name[i][IB_DEVICE_NAME_MAX], eq_name[i]);

why is the address you want at the position IB_DEVICE_NAME_MAX instead
of at index 0?  Also (this is theoretical only since IB_DEVICE_NAME_MAX
is much bigger than the size of "mthcaX") without range checking, since
you only allocate IB_DEVICE_NAME_MAX what prevents the eq_name part from
overflowing?  In general I don't like since strcpy()/strcat() instead of
strlcpy()/strlcat().

(And why write this as strcpy followed by strcat instead of a single
snprintf()?)

 - R.