Re: [PATCH v2] scsi: mvsas: Try to enable MSI

[John Garry <john.g.garry@oracle.com>]

On 06/11/2023 12:59, Marek Vasut wrote:
>>> drivers/scsi/mvsas/mv_init.c | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
>>> index 43ebb331e2167..d3b1cee6b3252 100644
>>> --- a/drivers/scsi/mvsas/mv_init.c
>>> +++ b/drivers/scsi/mvsas/mv_init.c
>>> @@ -571,6 +571,17 @@ static int mvs_pci_init(struct pci_dev *pdev, 
>>> const struct pci_device_id *ent)
>>>       rc = sas_register_ha(SHOST_TO_SAS_HA(shost));
>>>       if (rc)
>>>           goto err_out_shost;
>>> +
>>> +    /* Try to enable MSI, this is needed at least on OCZ RevoDrive 3 
>>> X2 */
>>> +    if (pdev->vendor == PCI_VENDOR_ID_OCZ) {
>>
>> PCI_VENDOR_ID_OCZ means 9485.

I meant chip_9485, not the PCI vendor ID. See how it is used as a lookup 
to chip-specific parameters for multiple OCZ and MARVELL SoCs in 
mvs_pci_table[] and mvs_chips[]

> 
> It does not, see:
> 
> $ git grep PCI_VENDOR_ID_OCZ include/
> include/linux/pci_ids.h:#define PCI_VENDOR_ID_OCZ               0x1b85
> 
>> So how about enable MSI for all PCI device IDs which use that, which 
>> is all OCZ and MARVELL_EXT? I could not get my hands on a datasheet 
>> for that SoC (could you?), but since all previous generations 
>> supported MSI, I think that it's a safe bet.
> 
> Nope. I only have the one device here.

Checking whether the PCI vendor is PCI_VENDOR_ID_OCZ actually covers 
many PCI devices, but they all use chip_9485

> 
>> Then, if we do that, instead of repeating this same vendor check, how 
>> about add a new member to mvs_chip_info to flag whether we need to try 
>> MSI? For example, it could be mvs_chip_info.use_msi .
>>
>>> +        rc = pci_enable_msi(mvi->pdev);
>>> +        if (rc) {
>>> +            dev_err(&mvi->pdev->dev,
>>> +                "mvsas: Failed to enable MSI for OCZ device, 
>>> attached drives may not be detected. rc=%d\n",
>>> +                rc);
>>
>> We should fail to load the driver in this case.
> 
> Wouldn't it be better to give the legacy IRQ a chance in any case, maybe 
> those do work on some of the other OCZ devices (or other versions of 
> firmware) ?

Then according to the change here, we would always call 
pci_disable_msi() in removal path for OCZ, regardless of whether the 
original pci_enable_msi() call was successful - is that safe and proper?

Thanks,
John