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
