On Fri, 25 Oct 2024 11:26:18 +0200,
Philipp Stanner wrote:
>
> Hi,
>
> On Thu, 2024-10-24 at 17:55 +0200, Takashi Iwai wrote:
> > pcim_intx() tries to restore the INTX_DISABLE bit at removal via
> > devres, but there is a chance that it restores a wrong value.
> > Because the value to be restored is blindly assumed to be the
> > negative
> > of the enable argument, when a driver calls pcim_intx() unnecessarily
> > for the already enabled state, it'll restore to the disabled state in
> > turn.
>
> It depends on how it is called, no?
>
> // INTx == 1
> pcim_intx(pdev, 0); // old INTx value assumed to be 1 -> correct
>
> ---
>
> // INTx == 0
> pcim_intx(pdev, 0); // old INTx value assumed to be 1 -> wrong
>
> Maybe it makes sense to replace part of the commit text with something
> like the example above?
If it helps better understanding, why not.
> > Also, when a driver calls pcim_intx() multiple times with
> > different enable argument values, the last one will win no matter
> > what
> > value it is.
>
> Means
>
> // INTx == 0
> pcim_intx(pdev, 0); // orig_INTx == 1, INTx == 0
> pcim_intx(pdev, 1); // orig_INTx == 0, INTx == 1
> pcim_intx(pdev, 0); // orig_INTx == 1, INTx == 0
>
> So in this example the first call would cause a wrong orig_INTx, but
> the last call – the one "who will win" – seems to do the right thing,
> dosen't it?
Yes and no. The last call wins to write the current value, but
shouldn't win for setting the original value. The original value must
be recorded only from the first call.
> > This patch addresses those inconsistencies by saving the original
> > INTX_DISABLE state at the first devres_alloc(); this assures that the
> > original state is restored properly, and the later pcim_intx() calls
> > won't overwrite res->orig_intx any longer.
> >
> > Fixes: 25216afc9db5 ("PCI: Add managed pcim_intx()")
>
> That commit is also in 6.11, so we need:
>
> Cc: stable@xxxxxxxxxxxxxxx # 6.11+
OK.
> > Link: https://lore.kernel.org/87v7xk2ps5.wl-tiwai@xxxxxxx
> > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> > ---
> > drivers/pci/devres.c | 18 ++++++++++++++----
> > 1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> > index b133967faef8..aed3c9a355cb 100644
> > --- 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, struct
> > pcim_intx_devres *res)
> > {
> > + 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, res);
>
> This is not the correct place – get_or_create_intx_devres() should get
> the resource if it exists, or allocate it if it doesn't, but its
> purpose is not to modify the resource.
The behavior of the function makes the implementation a bit harder,
because the initialization of res->orig_intx should be done only once
at the very first call.
> > 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;
>
> Here is the right place to call save_orig_intx(). That way you also
> won't need the new variable struct device *dev above :)
The problem is that, at this place, we don't know whether it's a
freshly created devres or it's an inherited one. So, we'd need to
modify get_or_create_intx_devres() to indicate that it's a new
creation. Or, maybe simpler would be rather to flatten
get_or_create_intx_devres() into pcim_intx(). It's a small function,
and it wouldn't be worsen the readability so much.
That is, something like below.
thanks,
Takashi
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -438,21 +438,6 @@ 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)
-{
- struct pcim_intx_devres *res;
-
- res = devres_find(dev, pcim_intx_restore, NULL, NULL);
- if (res)
- return res;
-
- res = devres_alloc(pcim_intx_restore, sizeof(*res), GFP_KERNEL);
- if (res)
- devres_add(dev, res);
-
- return res;
-}
-
/**
* pcim_intx - managed pci_intx()
* @pdev: the PCI device to operate on
@@ -466,12 +451,21 @@ static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev)
int pcim_intx(struct pci_dev *pdev, int enable)
{
struct pcim_intx_devres *res;
+ struct device *dev = &pdev->dev;
+ u16 pci_command;
- res = get_or_create_intx_devres(&pdev->dev);
- if (!res)
- return -ENOMEM;
+ res = devres_find(dev, pcim_intx_restore, NULL, NULL);
+ if (!res) {
+ res = devres_alloc(pcim_intx_restore, sizeof(*res), GFP_KERNEL);
+ if (!res)
+ return -ENOMEM;
+
+ pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
+ res->orig_intx = !(pci_command & PCI_COMMAND_INTX_DISABLE);
+
+ devres_add(dev, res);
+ }
- res->orig_intx = !enable;
__pcim_intx(pdev, enable);
return 0;
