* Re: [PATCH V2] simplify init function
2025-01-14 1:03 ` Damien Le Moal
@ 2025-01-14 9:00 ` Niklas Cassel
2025-01-14 18:04 ` Tomas Henzl
1 sibling, 0 replies; 4+ messages in thread
From: Niklas Cassel @ 2025-01-14 9:00 UTC (permalink / raw)
To: Damien Le Moal; +Cc: Tomas Henzl, linux-ide, Philipp Stanner
On Tue, Jan 14, 2025 at 10:03:44AM +0900, Damien Le Moal wrote:
> On 1/14/25 01:34, Tomas Henzl wrote:
> > by removing few lines. No functional change.
>
> Looks OK to me, but:
>
> The patch title should be: "ata: ahci: simplify init function"
>
> And the commit message should also describe why it is OK to change
> ahci_init_irq() to be a void function. Can you send a V3 with that addressed
> please ?
I assume that this simplification is possible because sending in flags
(PCI_IRQ_MSIX | PCI_IRQ_INTX) to pci_alloc_irq_vectors() guarantees that
it will first try to allocate MSIX, if so, that should have be mentioned
in the commit message.
Also, you are just replacing pci_intx() with
pci_alloc_irq_vectors(.., PCI_IRQ_INTX), is this always safe?
If so, it should have been mentioned in the commit message.
pci_intx() is a hybrid function which can sometimes be managed through
devres. To remove this hybrid nature from pci_intx(), Philip has written
patches that ports users to either an always-managed or a never-managed
version.
A libata patch for this is currently queued via the PCI tree (pci/next),
see linux-next:
https://lore.kernel.org/linux-ide/Z1vFWyHkBD4d5xnG@ryzen/T/#mc20794f1e8712dafe77bc64c8da44c4d9b227b28
https://lore.kernel.org/linux-ide/Z1vFWyHkBD4d5xnG@ryzen/T/#m3a42ac564d97d10f8b1924c675efd144c845f8d5
Your patch conflicts with his series, which is already queued.
Kind regards,
Niklas
>
> >
> > Signed-off-by: Tomas Henzl <thenzl@redhat.com>
> > ---
> > V2: ahci_init_irq is now a void function
> >
> > drivers/ata/ahci.c | 23 ++++++++++++-----------
> > 1 file changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> > index 8d27c567be1c..3ea2f3adf354 100644
> > --- a/drivers/ata/ahci.c
> > +++ b/drivers/ata/ahci.c
> > @@ -1665,13 +1665,15 @@ static int ahci_get_irq_vector(struct ata_host *host, int port)
> > return pci_irq_vector(to_pci_dev(host->dev), port);
> > }
> >
> > -static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports,
> > +static void ahci_init_irq(struct pci_dev *pdev, unsigned int n_ports,
> > struct ahci_host_priv *hpriv)
> > {
> > int nvec;
> >
> > - if (hpriv->flags & AHCI_HFLAG_NO_MSI)
> > - return -ENODEV;
> > + if (hpriv->flags & AHCI_HFLAG_NO_MSI) {
> > + pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX);
> > + return;
> > + }
> >
> > /*
> > * If number of MSIs is less than number of ports then Sharing Last
> > @@ -1685,7 +1687,7 @@ static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports,
> > if (!(readl(hpriv->mmio + HOST_CTL) & HOST_MRSM)) {
> > hpriv->get_irq_vector = ahci_get_irq_vector;
> > hpriv->flags |= AHCI_HFLAG_MULTI_MSI;
> > - return nvec;
> > + return;
> > }
> >
> > /*
> > @@ -1700,12 +1702,13 @@ static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports,
> >
> > /*
> > * If the host is not capable of supporting per-port vectors, fall
> > - * back to single MSI before finally attempting single MSI-X.
> > + * back to single MSI before finally attempting single MSI-X or
> > + * a legacy INTx.
> > */
> > nvec = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> > if (nvec == 1)
> > - return nvec;
> > - return pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX);
> > + return;
> > + pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX | PCI_IRQ_INTX);
> > }
> >
> > static void ahci_mark_external_port(struct ata_port *ap)
> > @@ -1985,10 +1988,8 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> > }
> > host->private_data = hpriv;
> >
> > - if (ahci_init_msi(pdev, n_ports, hpriv) < 0) {
> > - /* legacy intx interrupts */
> > - pci_intx(pdev, 1);
> > - }
> > + ahci_init_irq(pdev, n_ports, hpriv);
> > +
> > hpriv->irq = pci_irq_vector(pdev, 0);
> >
> > if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
>
>
> --
> Damien Le Moal
> Western Digital Research
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH V2] simplify init function
2025-01-14 1:03 ` Damien Le Moal
2025-01-14 9:00 ` Niklas Cassel
@ 2025-01-14 18:04 ` Tomas Henzl
1 sibling, 0 replies; 4+ messages in thread
From: Tomas Henzl @ 2025-01-14 18:04 UTC (permalink / raw)
To: Damien Le Moal, linux-ide; +Cc: Niklas.Cassel
On 1/14/25 2:03 AM, Damien Le Moal wrote:
> On 1/14/25 01:34, Tomas Henzl wrote:
>> by removing few lines. No functional change.
>
> Looks OK to me, but:
>
> The patch title should be: "ata: ahci: simplify init function"
>
> And the commit message should also describe why it is OK to change
> ahci_init_irq() to be a void function. Can you send a V3 with that addressed
> please ?
sure
>
>>
>> Signed-off-by: Tomas Henzl <thenzl@redhat.com>
>> ---
>> V2: ahci_init_irq is now a void function
>>
>> drivers/ata/ahci.c | 23 ++++++++++++-----------
>> 1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index 8d27c567be1c..3ea2f3adf354 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -1665,13 +1665,15 @@ static int ahci_get_irq_vector(struct ata_host *host, int port)
>> return pci_irq_vector(to_pci_dev(host->dev), port);
>> }
>>
>> -static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports,
>> +static void ahci_init_irq(struct pci_dev *pdev, unsigned int n_ports,
>> struct ahci_host_priv *hpriv)
>> {
>> int nvec;
>>
>> - if (hpriv->flags & AHCI_HFLAG_NO_MSI)
>> - return -ENODEV;
>> + if (hpriv->flags & AHCI_HFLAG_NO_MSI) {
>> + pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX);
>> + return;
>> + }
>>
>> /*
>> * If number of MSIs is less than number of ports then Sharing Last
>> @@ -1685,7 +1687,7 @@ static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports,
>> if (!(readl(hpriv->mmio + HOST_CTL) & HOST_MRSM)) {
>> hpriv->get_irq_vector = ahci_get_irq_vector;
>> hpriv->flags |= AHCI_HFLAG_MULTI_MSI;
>> - return nvec;
>> + return;
>> }
>>
>> /*
>> @@ -1700,12 +1702,13 @@ static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports,
>>
>> /*
>> * If the host is not capable of supporting per-port vectors, fall
>> - * back to single MSI before finally attempting single MSI-X.
>> + * back to single MSI before finally attempting single MSI-X or
>> + * a legacy INTx.
>> */
>> nvec = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
>> if (nvec == 1)
>> - return nvec;
>> - return pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX);
>> + return;
>> + pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX | PCI_IRQ_INTX);
>> }
>>
>> static void ahci_mark_external_port(struct ata_port *ap)
>> @@ -1985,10 +1988,8 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>> }
>> host->private_data = hpriv;
>>
>> - if (ahci_init_msi(pdev, n_ports, hpriv) < 0) {
>> - /* legacy intx interrupts */
>> - pci_intx(pdev, 1);
>> - }
>> + ahci_init_irq(pdev, n_ports, hpriv);
>> +
>> hpriv->irq = pci_irq_vector(pdev, 0);
>>
>> if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread