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

[Damien Le Moal <dlemoal@kernel.org>]

On 11/6/23 23:12, John Garry wrote:
> 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?

pci_disable_msi() does nothing if MSI is not enabled. So it shouldn't be an issue.

> 
> Thanks,
> John
> 
> 
> 

-- 
Damien Le Moal
Western Digital Research