> 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. _______________________________________________ general mailing list general@lists.openfabrics.org http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general