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

[Marek Vasut <marex@denx.de>]

On 11/6/23 12:08, John Garry wrote:

Hi,

>>   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.

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.

> 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) ?