pci_intx() is a hybrid function which can sometimes be managed through
devres. To remove this hybrid nature from pci_intx(), it is necessary to
port users to either an always-managed or a never-managed version.
hda_intel enables its PCI-Device with pcim_enable_device(). Thus, it needs
the always-managed version.
Replace pci_intx() with pcim_intx().
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
sound/pci/hda/hda_intel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index b4540c5cd2a6..b44ca7b6e54f 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx *chip, int do_disconnect)
}
bus->irq = chip->pci->irq;
chip->card->sync_irq = bus->irq;
- pci_intx(chip->pci, !chip->msi);
+ pcim_intx(chip->pci, !chip->msi);
return 0;
}
--
2.47.0On Tue, 15 Oct 2024 20:51:12 +0200, Philipp Stanner wrote: > > pci_intx() is a hybrid function which can sometimes be managed through > devres. To remove this hybrid nature from pci_intx(), it is necessary to > port users to either an always-managed or a never-managed version. > > hda_intel enables its PCI-Device with pcim_enable_device(). Thus, it needs > the always-managed version. > > Replace pci_intx() with pcim_intx(). > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > --- > sound/pci/hda/hda_intel.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > index b4540c5cd2a6..b44ca7b6e54f 100644 > --- a/sound/pci/hda/hda_intel.c > +++ b/sound/pci/hda/hda_intel.c > @@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx *chip, int do_disconnect) > } > bus->irq = chip->pci->irq; > chip->card->sync_irq = bus->irq; > - pci_intx(chip->pci, !chip->msi); > + pcim_intx(chip->pci, !chip->msi); > return 0; > } > Hm, it's OK-ish to do this as it's practically same as what pci_intx() currently does. But, the current code can be a bit inconsistent about the original intx value. pcim_intx() always stores !enable to res->orig_intx unconditionally, and it means that the orig_intx value gets overridden at each time pcim_intx() gets called. Meanwhile, HD-audio driver does release and re-acquire the interrupt after disabling MSI when something goes wrong, and pci_intx() call above is a part of that procedure. So, it can rewrite the res->orig_intx to another value by retry without MSI. And after the driver removal, it'll lead to another state. In anyway, as it doesn't change the current behavior, feel free to take my ack for now: Acked-by: Takashi Iwai <tiwai@suse.de> thanks, Takashi
On Tue, 2024-10-22 at 16:08 +0200, Takashi Iwai wrote: > On Tue, 15 Oct 2024 20:51:12 +0200, > Philipp Stanner wrote: > > > > pci_intx() is a hybrid function which can sometimes be managed > > through > > devres. To remove this hybrid nature from pci_intx(), it is > > necessary to > > port users to either an always-managed or a never-managed version. > > > > hda_intel enables its PCI-Device with pcim_enable_device(). Thus, > > it needs > > the always-managed version. > > > > Replace pci_intx() with pcim_intx(). > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > --- > > sound/pci/hda/hda_intel.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > > index b4540c5cd2a6..b44ca7b6e54f 100644 > > --- a/sound/pci/hda/hda_intel.c > > +++ b/sound/pci/hda/hda_intel.c > > @@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx *chip, > > int do_disconnect) > > } > > bus->irq = chip->pci->irq; > > chip->card->sync_irq = bus->irq; > > - pci_intx(chip->pci, !chip->msi); > > + pcim_intx(chip->pci, !chip->msi); > > return 0; > > } > > > > Hm, it's OK-ish to do this as it's practically same as what > pci_intx() > currently does. But, the current code can be a bit inconsistent > about > the original intx value. pcim_intx() always stores !enable to > res->orig_intx unconditionally, and it means that the orig_intx value > gets overridden at each time pcim_intx() gets called. Yes. > > Meanwhile, HD-audio driver does release and re-acquire the interrupt > after disabling MSI when something goes wrong, and pci_intx() call > above is a part of that procedure. So, it can rewrite the > res->orig_intx to another value by retry without MSI. And after the > driver removal, it'll lead to another state. I'm not sure that I understand this paragraph completely. Still, could a solution for the driver on the long-term just be to use pci_intx()? > > In anyway, as it doesn't change the current behavior, feel free to > take my ack for now: > > Acked-by: Takashi Iwai <tiwai@suse.de> Thank you, P. > > > thanks, > > Takashi >
On Wed, 23 Oct 2024 15:50:09 +0200,
Philipp Stanner wrote:
>
> On Tue, 2024-10-22 at 16:08 +0200, Takashi Iwai wrote:
> > On Tue, 15 Oct 2024 20:51:12 +0200,
> > Philipp Stanner wrote:
> > >
> > > pci_intx() is a hybrid function which can sometimes be managed
> > > through
> > > devres. To remove this hybrid nature from pci_intx(), it is
> > > necessary to
> > > port users to either an always-managed or a never-managed version.
> > >
> > > hda_intel enables its PCI-Device with pcim_enable_device(). Thus,
> > > it needs
> > > the always-managed version.
> > >
> > > Replace pci_intx() with pcim_intx().
> > >
> > > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > > ---
> > > sound/pci/hda/hda_intel.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > index b4540c5cd2a6..b44ca7b6e54f 100644
> > > --- a/sound/pci/hda/hda_intel.c
> > > +++ b/sound/pci/hda/hda_intel.c
> > > @@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx *chip,
> > > int do_disconnect)
> > > }
> > > bus->irq = chip->pci->irq;
> > > chip->card->sync_irq = bus->irq;
> > > - pci_intx(chip->pci, !chip->msi);
> > > + pcim_intx(chip->pci, !chip->msi);
> > > return 0;
> > > }
> > >
> >
> > Hm, it's OK-ish to do this as it's practically same as what
> > pci_intx()
> > currently does. But, the current code can be a bit inconsistent
> > about
> > the original intx value. pcim_intx() always stores !enable to
> > res->orig_intx unconditionally, and it means that the orig_intx value
> > gets overridden at each time pcim_intx() gets called.
>
> Yes.
>
> >
> > Meanwhile, HD-audio driver does release and re-acquire the interrupt
> > after disabling MSI when something goes wrong, and pci_intx() call
> > above is a part of that procedure. So, it can rewrite the
> > res->orig_intx to another value by retry without MSI. And after the
> > driver removal, it'll lead to another state.
>
> I'm not sure that I understand this paragraph completely. Still, could
> a solution for the driver on the long-term just be to use pci_intx()?
pci_intx() misses the restore of the original value, so it's no
long-term solution, either.
What I meant is that pcim_intx() blindly assumes the negative of the
passed argument as the original state, which isn't always true. e.g.
when the driver calls it twice with different values, a wrong value
may be remembered.
That said, I thought of something like below.
thanks,
Takashi
-- 8< --
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -438,8 +438,17 @@ static void pcim_intx_restore(struct device *dev, void *data)
__pcim_intx(pdev, res->orig_intx);
}
-static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
+static void save_orig_intx(struct pci_dev *pdev)
{
+ u16 pci_command;
+
+ pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
+ res->orig_intx = !(pci_command & PCI_COMMAND_INTX_DISABLE);
+}
+
+static struct pcim_intx_devres *get_or_create_intx_devres(struct pci_dev *pdev)
+{
+ struct device *dev = &pdev->dev;
struct pcim_intx_devres *res;
res = devres_find(dev, pcim_intx_restore, NULL, NULL);
@@ -447,8 +456,10 @@ static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
return res;
res = devres_alloc(pcim_intx_restore, sizeof(*res), GFP_KERNEL);
- if (res)
+ if (res) {
+ save_orig_intx(pdev);
devres_add(dev, res);
+ }
return res;
}
@@ -467,11 +478,10 @@ int pcim_intx(struct pci_dev *pdev, int enable)
{
struct pcim_intx_devres *res;
- res = get_or_create_intx_devres(&pdev->dev);
+ res = get_or_create_intx_devres(pdev);
if (!res)
return -ENOMEM;
- res->orig_intx = !enable;
__pcim_intx(pdev, enable);
return 0;
On Wed, 2024-10-23 at 17:03 +0200, Takashi Iwai wrote:
> On Wed, 23 Oct 2024 15:50:09 +0200,
> Philipp Stanner wrote:
> >
> > On Tue, 2024-10-22 at 16:08 +0200, Takashi Iwai wrote:
> > > On Tue, 15 Oct 2024 20:51:12 +0200,
> > > Philipp Stanner wrote:
> > > >
> > > > pci_intx() is a hybrid function which can sometimes be managed
> > > > through
> > > > devres. To remove this hybrid nature from pci_intx(), it is
> > > > necessary to
> > > > port users to either an always-managed or a never-managed
> > > > version.
> > > >
> > > > hda_intel enables its PCI-Device with pcim_enable_device().
> > > > Thus,
> > > > it needs
> > > > the always-managed version.
> > > >
> > > > Replace pci_intx() with pcim_intx().
> > > >
> > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > > > ---
> > > > sound/pci/hda/hda_intel.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/sound/pci/hda/hda_intel.c
> > > > b/sound/pci/hda/hda_intel.c
> > > > index b4540c5cd2a6..b44ca7b6e54f 100644
> > > > --- a/sound/pci/hda/hda_intel.c
> > > > +++ b/sound/pci/hda/hda_intel.c
> > > > @@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx
> > > > *chip,
> > > > int do_disconnect)
> > > > }
> > > > bus->irq = chip->pci->irq;
> > > > chip->card->sync_irq = bus->irq;
> > > > - pci_intx(chip->pci, !chip->msi);
> > > > + pcim_intx(chip->pci, !chip->msi);
> > > > return 0;
> > > > }
> > > >
> > >
> > > Hm, it's OK-ish to do this as it's practically same as what
> > > pci_intx()
> > > currently does. But, the current code can be a bit inconsistent
> > > about
> > > the original intx value. pcim_intx() always stores !enable to
> > > res->orig_intx unconditionally, and it means that the orig_intx
> > > value
> > > gets overridden at each time pcim_intx() gets called.
> >
> > Yes.
> >
> > >
> > > Meanwhile, HD-audio driver does release and re-acquire the
> > > interrupt
> > > after disabling MSI when something goes wrong, and pci_intx()
> > > call
> > > above is a part of that procedure. So, it can rewrite the
> > > res->orig_intx to another value by retry without MSI. And after
> > > the
> > > driver removal, it'll lead to another state.
> >
> > I'm not sure that I understand this paragraph completely. Still,
> > could
> > a solution for the driver on the long-term just be to use
> > pci_intx()?
>
> pci_intx() misses the restore of the original value, so it's no
> long-term solution, either.
Sure that is missing – I was basically asking whether the driver could
live without that feature.
Consider that point obsolete, see below
>
> What I meant is that pcim_intx() blindly assumes the negative of the
> passed argument as the original state, which isn't always true. e.g.
> when the driver calls it twice with different values, a wrong value
> may be remembered.
Ah, I see – thoguh the issue is when it's called several times with the
*same* value, isn't it?
E.g.
pcim_intx(pdev, 1); // 0 is remembered as the old value
pcim_intx(pdev, 1); // 0 is falsely remembered as the old value
Also, it would seem that calling the function for the first time like
that:
pcim_intx(pdev, 0); // old value: 1
is at least incorrect, because INTx should be 0 per default, shouldn't
it? Could then even be a 1st class bug, because INTx would end up being
enabled despite having been disabled all the time.
>
> That said, I thought of something like below.
At first glance that looks like a good idea to me, thanks for working
this out!
IMO you can submit that as a patch so we can discuss it separately.
Greetings,
Philipp
>
>
> thanks,
>
> Takashi
>
> -- 8< --
> --- a/drivers/pci/devres.c
> +++ b/drivers/pci/devres.c
> @@ -438,8 +438,17 @@ static void pcim_intx_restore(struct device
> *dev, void *data)
> __pcim_intx(pdev, res->orig_intx);
> }
>
> -static struct pcim_intx_devres *get_or_create_intx_devres(struct
> device *dev)
> +static void save_orig_intx(struct pci_dev *pdev)
> {
> + u16 pci_command;
> +
> + pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
> + res->orig_intx = !(pci_command & PCI_COMMAND_INTX_DISABLE);
> +}
> +
> +static struct pcim_intx_devres *get_or_create_intx_devres(struct
> pci_dev *pdev)
> +{
> + struct device *dev = &pdev->dev;
> struct pcim_intx_devres *res;
>
> res = devres_find(dev, pcim_intx_restore, NULL, NULL);
> @@ -447,8 +456,10 @@ static struct pcim_intx_devres
> *get_or_create_intx_devres(struct device *dev)
> return res;
>
> res = devres_alloc(pcim_intx_restore, sizeof(*res),
> GFP_KERNEL);
> - if (res)
> + if (res) {
> + save_orig_intx(pdev);
> devres_add(dev, res);
> + }
>
> return res;
> }
> @@ -467,11 +478,10 @@ int pcim_intx(struct pci_dev *pdev, int enable)
> {
> struct pcim_intx_devres *res;
>
> - res = get_or_create_intx_devres(&pdev->dev);
> + res = get_or_create_intx_devres(pdev);
> if (!res)
> return -ENOMEM;
>
> - res->orig_intx = !enable;
> __pcim_intx(pdev, enable);
>
> return 0;
>
On Thu, 24 Oct 2024 10:02:59 +0200, Philipp Stanner wrote: > > On Wed, 2024-10-23 at 17:03 +0200, Takashi Iwai wrote: > > On Wed, 23 Oct 2024 15:50:09 +0200, > > Philipp Stanner wrote: > > > > > > On Tue, 2024-10-22 at 16:08 +0200, Takashi Iwai wrote: > > > > On Tue, 15 Oct 2024 20:51:12 +0200, > > > > Philipp Stanner wrote: > > > > > > > > > > pci_intx() is a hybrid function which can sometimes be managed > > > > > through > > > > > devres. To remove this hybrid nature from pci_intx(), it is > > > > > necessary to > > > > > port users to either an always-managed or a never-managed > > > > > version. > > > > > > > > > > hda_intel enables its PCI-Device with pcim_enable_device(). > > > > > Thus, > > > > > it needs > > > > > the always-managed version. > > > > > > > > > > Replace pci_intx() with pcim_intx(). > > > > > > > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > > > > --- > > > > > sound/pci/hda/hda_intel.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/sound/pci/hda/hda_intel.c > > > > > b/sound/pci/hda/hda_intel.c > > > > > index b4540c5cd2a6..b44ca7b6e54f 100644 > > > > > --- a/sound/pci/hda/hda_intel.c > > > > > +++ b/sound/pci/hda/hda_intel.c > > > > > @@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx > > > > > *chip, > > > > > int do_disconnect) > > > > > } > > > > > bus->irq = chip->pci->irq; > > > > > chip->card->sync_irq = bus->irq; > > > > > - pci_intx(chip->pci, !chip->msi); > > > > > + pcim_intx(chip->pci, !chip->msi); > > > > > return 0; > > > > > } > > > > > > > > > > > > > Hm, it's OK-ish to do this as it's practically same as what > > > > pci_intx() > > > > currently does. But, the current code can be a bit inconsistent > > > > about > > > > the original intx value. pcim_intx() always stores !enable to > > > > res->orig_intx unconditionally, and it means that the orig_intx > > > > value > > > > gets overridden at each time pcim_intx() gets called. > > > > > > Yes. > > > > > > > > > > > Meanwhile, HD-audio driver does release and re-acquire the > > > > interrupt > > > > after disabling MSI when something goes wrong, and pci_intx() > > > > call > > > > above is a part of that procedure. So, it can rewrite the > > > > res->orig_intx to another value by retry without MSI. And after > > > > the > > > > driver removal, it'll lead to another state. > > > > > > I'm not sure that I understand this paragraph completely. Still, > > > could > > > a solution for the driver on the long-term just be to use > > > pci_intx()? > > > > pci_intx() misses the restore of the original value, so it's no > > long-term solution, either. > > Sure that is missing – I was basically asking whether the driver could > live without that feature. > > Consider that point obsolete, see below > > > > > What I meant is that pcim_intx() blindly assumes the negative of the > > passed argument as the original state, which isn't always true. e.g. > > when the driver calls it twice with different values, a wrong value > > may be remembered. > > Ah, I see – thoguh the issue is when it's called several times with the > *same* value, isn't it? > > E.g. > > pcim_intx(pdev, 1); // 0 is remembered as the old value > pcim_intx(pdev, 1); // 0 is falsely remembered as the old value > > Also, it would seem that calling the function for the first time like > that: > > pcim_intx(pdev, 0); // old value: 1 > > is at least incorrect, because INTx should be 0 per default, shouldn't > it? Could then even be a 1st class bug, because INTx would end up being > enabled despite having been disabled all the time. Yeah, and the unexpected restore can happen even with a single call of pcim_intx(), if the driver calls it unnecessarily. > > That said, I thought of something like below. > > At first glance that looks like a good idea to me, thanks for working > this out! > > IMO you can submit that as a patch so we can discuss it separately. Sure, I'm going to submit later. thanks, Takashi
On Thu, 2024-10-24 at 17:43 +0200, Takashi Iwai wrote:
> On Thu, 24 Oct 2024 10:02:59 +0200,
> Philipp Stanner wrote:
> >
> > On Wed, 2024-10-23 at 17:03 +0200, Takashi Iwai wrote:
> > > On Wed, 23 Oct 2024 15:50:09 +0200,
> > > Philipp Stanner wrote:
> > > >
> > > > On Tue, 2024-10-22 at 16:08 +0200, Takashi Iwai wrote:
> > > > > On Tue, 15 Oct 2024 20:51:12 +0200,
> > > > > Philipp Stanner wrote:
> > > > > >
> > > > > > pci_intx() is a hybrid function which can sometimes be
> > > > > > managed
> > > > > > through
> > > > > > devres. To remove this hybrid nature from pci_intx(), it is
> > > > > > necessary to
> > > > > > port users to either an always-managed or a never-managed
> > > > > > version.
> > > > > >
> > > > > > hda_intel enables its PCI-Device with pcim_enable_device().
> > > > > > Thus,
> > > > > > it needs
> > > > > > the always-managed version.
> > > > > >
> > > > > > Replace pci_intx() with pcim_intx().
> > > > > >
> > > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > > > > > ---
> > > > > > sound/pci/hda/hda_intel.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/sound/pci/hda/hda_intel.c
> > > > > > b/sound/pci/hda/hda_intel.c
> > > > > > index b4540c5cd2a6..b44ca7b6e54f 100644
> > > > > > --- a/sound/pci/hda/hda_intel.c
> > > > > > +++ b/sound/pci/hda/hda_intel.c
> > > > > > @@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx
> > > > > > *chip,
> > > > > > int do_disconnect)
> > > > > > }
> > > > > > bus->irq = chip->pci->irq;
> > > > > > chip->card->sync_irq = bus->irq;
> > > > > > - pci_intx(chip->pci, !chip->msi);
> > > > > > + pcim_intx(chip->pci, !chip->msi);
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > >
> > > > > Hm, it's OK-ish to do this as it's practically same as what
> > > > > pci_intx()
> > > > > currently does. But, the current code can be a bit
> > > > > inconsistent
> > > > > about
> > > > > the original intx value. pcim_intx() always stores !enable
> > > > > to
> > > > > res->orig_intx unconditionally, and it means that the
> > > > > orig_intx
> > > > > value
> > > > > gets overridden at each time pcim_intx() gets called.
> > > >
> > > > Yes.
> > > >
> > > > >
> > > > > Meanwhile, HD-audio driver does release and re-acquire the
> > > > > interrupt
> > > > > after disabling MSI when something goes wrong, and pci_intx()
> > > > > call
> > > > > above is a part of that procedure. So, it can rewrite the
> > > > > res->orig_intx to another value by retry without MSI. And
> > > > > after
> > > > > the
> > > > > driver removal, it'll lead to another state.
> > > >
> > > > I'm not sure that I understand this paragraph completely.
> > > > Still,
> > > > could
> > > > a solution for the driver on the long-term just be to use
> > > > pci_intx()?
> > >
> > > pci_intx() misses the restore of the original value, so it's no
> > > long-term solution, either.
> >
> > Sure that is missing – I was basically asking whether the driver
> > could
> > live without that feature.
> >
> > Consider that point obsolete, see below
> >
> > >
> > > What I meant is that pcim_intx() blindly assumes the negative of
> > > the
> > > passed argument as the original state, which isn't always true.
> > > e.g.
> > > when the driver calls it twice with different values, a wrong
> > > value
> > > may be remembered.
> >
> > Ah, I see – thoguh the issue is when it's called several times with
> > the
> > *same* value, isn't it?
> >
> > E.g.
> >
> > pcim_intx(pdev, 1); // 0 is remembered as the old value
> > pcim_intx(pdev, 1); // 0 is falsely remembered as the old value
> >
> > Also, it would seem that calling the function for the first time
> > like
> > that:
> >
> > pcim_intx(pdev, 0); // old value: 1
> >
> > is at least incorrect, because INTx should be 0 per default,
> > shouldn't
> > it? Could then even be a 1st class bug, because INTx would end up
> > being
> > enabled despite having been disabled all the time.
>
> Yeah, and the unexpected restore can happen even with a single call
> of
> pcim_intx(), if the driver calls it unnecessarily.
>
> > > That said, I thought of something like below.
> >
> > At first glance that looks like a good idea to me, thanks for
> > working
> > this out!
> >
> > IMO you can submit that as a patch so we can discuss it separately.
>
> Sure, I'm going to submit later.
I just took a look into the old implementation of pci_intx() (there was
no pcim_intx() back then), before I started cleaning up PCI's devres.
This what it looked like before
25216afc9db53d85dc648aba8fb7f6d31f2c8731:
void pci_intx(struct pci_dev *pdev, int enable)
{
u16 pci_command, new;
pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
if (enable)
new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
else
new = pci_command | PCI_COMMAND_INTX_DISABLE;
if (new != pci_command) {
struct pci_devres *dr;
pci_write_config_word(pdev, PCI_COMMAND, new);
dr = find_pci_dr(pdev);
if (dr && !dr->restore_intx) {
dr->restore_intx = 1;
dr->orig_intx = !enable;
}
}
}
EXPORT_SYMBOL_GPL(pci_intx);
If I'm not mistaken the old version did not have the problem because
the value to be restored only changed if new != pci_command.
That should always be correct, what do you think?
If so, only my commit 25216afc9db53d85dc648aba8fb7f6d31f2c8731 needs to
be fixed.
Thanks,
P.
>
>
> thanks,
>
> Takashi
>
On Fri, 25 Oct 2024 10:37:57 +0200,
Philipp Stanner wrote:
>
> On Thu, 2024-10-24 at 17:43 +0200, Takashi Iwai wrote:
> > On Thu, 24 Oct 2024 10:02:59 +0200,
> > Philipp Stanner wrote:
> > >
> > > On Wed, 2024-10-23 at 17:03 +0200, Takashi Iwai wrote:
> > > > On Wed, 23 Oct 2024 15:50:09 +0200,
> > > > Philipp Stanner wrote:
> > > > >
> > > > > On Tue, 2024-10-22 at 16:08 +0200, Takashi Iwai wrote:
> > > > > > On Tue, 15 Oct 2024 20:51:12 +0200,
> > > > > > Philipp Stanner wrote:
> > > > > > >
> > > > > > > pci_intx() is a hybrid function which can sometimes be
> > > > > > > managed
> > > > > > > through
> > > > > > > devres. To remove this hybrid nature from pci_intx(), it is
> > > > > > > necessary to
> > > > > > > port users to either an always-managed or a never-managed
> > > > > > > version.
> > > > > > >
> > > > > > > hda_intel enables its PCI-Device with pcim_enable_device().
> > > > > > > Thus,
> > > > > > > it needs
> > > > > > > the always-managed version.
> > > > > > >
> > > > > > > Replace pci_intx() with pcim_intx().
> > > > > > >
> > > > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > > > > > > ---
> > > > > > > sound/pci/hda/hda_intel.c | 2 +-
> > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/sound/pci/hda/hda_intel.c
> > > > > > > b/sound/pci/hda/hda_intel.c
> > > > > > > index b4540c5cd2a6..b44ca7b6e54f 100644
> > > > > > > --- a/sound/pci/hda/hda_intel.c
> > > > > > > +++ b/sound/pci/hda/hda_intel.c
> > > > > > > @@ -786,7 +786,7 @@ static int azx_acquire_irq(struct azx
> > > > > > > *chip,
> > > > > > > int do_disconnect)
> > > > > > > }
> > > > > > > bus->irq = chip->pci->irq;
> > > > > > > chip->card->sync_irq = bus->irq;
> > > > > > > - pci_intx(chip->pci, !chip->msi);
> > > > > > > + pcim_intx(chip->pci, !chip->msi);
> > > > > > > return 0;
> > > > > > > }
> > > > > > >
> > > > > >
> > > > > > Hm, it's OK-ish to do this as it's practically same as what
> > > > > > pci_intx()
> > > > > > currently does. But, the current code can be a bit
> > > > > > inconsistent
> > > > > > about
> > > > > > the original intx value. pcim_intx() always stores !enable
> > > > > > to
> > > > > > res->orig_intx unconditionally, and it means that the
> > > > > > orig_intx
> > > > > > value
> > > > > > gets overridden at each time pcim_intx() gets called.
> > > > >
> > > > > Yes.
> > > > >
> > > > > >
> > > > > > Meanwhile, HD-audio driver does release and re-acquire the
> > > > > > interrupt
> > > > > > after disabling MSI when something goes wrong, and pci_intx()
> > > > > > call
> > > > > > above is a part of that procedure. So, it can rewrite the
> > > > > > res->orig_intx to another value by retry without MSI. And
> > > > > > after
> > > > > > the
> > > > > > driver removal, it'll lead to another state.
> > > > >
> > > > > I'm not sure that I understand this paragraph completely.
> > > > > Still,
> > > > > could
> > > > > a solution for the driver on the long-term just be to use
> > > > > pci_intx()?
> > > >
> > > > pci_intx() misses the restore of the original value, so it's no
> > > > long-term solution, either.
> > >
> > > Sure that is missing – I was basically asking whether the driver
> > > could
> > > live without that feature.
> > >
> > > Consider that point obsolete, see below
> > >
> > > >
> > > > What I meant is that pcim_intx() blindly assumes the negative of
> > > > the
> > > > passed argument as the original state, which isn't always true.
> > > > e.g.
> > > > when the driver calls it twice with different values, a wrong
> > > > value
> > > > may be remembered.
> > >
> > > Ah, I see – thoguh the issue is when it's called several times with
> > > the
> > > *same* value, isn't it?
> > >
> > > E.g.
> > >
> > > pcim_intx(pdev, 1); // 0 is remembered as the old value
> > > pcim_intx(pdev, 1); // 0 is falsely remembered as the old value
> > >
> > > Also, it would seem that calling the function for the first time
> > > like
> > > that:
> > >
> > > pcim_intx(pdev, 0); // old value: 1
> > >
> > > is at least incorrect, because INTx should be 0 per default,
> > > shouldn't
> > > it? Could then even be a 1st class bug, because INTx would end up
> > > being
> > > enabled despite having been disabled all the time.
> >
> > Yeah, and the unexpected restore can happen even with a single call
> > of
> > pcim_intx(), if the driver calls it unnecessarily.
> >
> > > > That said, I thought of something like below.
> > >
> > > At first glance that looks like a good idea to me, thanks for
> > > working
> > > this out!
> > >
> > > IMO you can submit that as a patch so we can discuss it separately.
> >
> > Sure, I'm going to submit later.
>
> I just took a look into the old implementation of pci_intx() (there was
> no pcim_intx() back then), before I started cleaning up PCI's devres.
> This what it looked like before
> 25216afc9db53d85dc648aba8fb7f6d31f2c8731:
>
> void pci_intx(struct pci_dev *pdev, int enable)
> {
> u16 pci_command, new;
>
> pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
>
> if (enable)
> new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
> else
> new = pci_command | PCI_COMMAND_INTX_DISABLE;
>
> if (new != pci_command) {
> struct pci_devres *dr;
>
> pci_write_config_word(pdev, PCI_COMMAND, new);
>
> dr = find_pci_dr(pdev);
> if (dr && !dr->restore_intx) {
> dr->restore_intx = 1;
> dr->orig_intx = !enable;
> }
> }
> }
> EXPORT_SYMBOL_GPL(pci_intx);
>
> If I'm not mistaken the old version did not have the problem because
> the value to be restored only changed if new != pci_command.
>
> That should always be correct, what do you think?
>
> If so, only my commit 25216afc9db53d85dc648aba8fb7f6d31f2c8731 needs to
> be fixed.
Yes, it looks so. Fortunately my submitted patch pointed to the right
Fixes tag :)
thanks,
Takashi
© 2016 - 2024 Red Hat, Inc.