| Message ID | 20241024155539.19416-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Restore the original INTX_DISABLE bit by pcim_intx() | expand |
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? > 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? > > 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@vger.kernel.org # 6.11+ > Link: https://lore.kernel.org/87v7xk2ps5.wl-tiwai@suse.de > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > 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. > 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 :) Thank you, P. > __pcim_intx(pdev, enable); > > return 0;
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@vger.kernel.org # 6.11+ OK. > > Link: https://lore.kernel.org/87v7xk2ps5.wl-tiwai@suse.de > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > 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;
On Fri, 2024-10-25 at 12:44 +0200, Takashi Iwai wrote: > 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. Alright, so you think that pcim_intx() should always restore the INTx state that existed before the driver was loaded. > > > 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@vger.kernel.org # 6.11+ > > OK. > > > > Link: https://lore.kernel.org/87v7xk2ps5.wl-tiwai@suse.de > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > --- > > > 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 might be the best solution. If it's done that way it should include a comment detailing the problem. Looking at the implementation of pci_intx() before 25216afc9db53d85dc648aba8fb7f6d31f2c8731 probably indicates that you're right: if (dr && !dr->restore_intx) { dr->restore_intx = 1; dr->orig_intx = !enable; } So they used a boolean to only take the first state. Although that still wouldn't have necessarily been the pre-driver INTx state. > > 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); sth like: /* * pcim_intx() must only restore the INTx value that existed before the * driver was loaded, i.e., before it called pcim_intx() for the * first time. */ > + 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); Looks like a good idea to me The only thing I'm wondering about right now is the following: In the old days, there was only pci_intx(), which either did devres or didn't. Now you have two functions, pcim_intx() and pci_intx(). The thing is that the driver could theoretically still intermingle them and for example call pci_intx() before pcim_intx(), which would lead the latter to still restore the wrong value. But that's very unlikely and I'm not sure whether we can do something about it. Regards, P. > > return 0; >
On Fri, 25 Oct 2024 16:28:42 +0200, Philipp Stanner wrote: > > On Fri, 2024-10-25 at 12:44 +0200, Takashi Iwai wrote: > > 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. > > Alright, so you think that pcim_intx() should always restore the INTx > state that existed before the driver was loaded. > > > > > 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@vger.kernel.org # 6.11+ > > > > OK. > > > > > > Link: https://lore.kernel.org/87v7xk2ps5.wl-tiwai@suse.de > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > > --- > > > > 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 might be the best solution. If it's done that way it should > include a comment detailing the problem. > > Looking at the implementation of pci_intx() before > 25216afc9db53d85dc648aba8fb7f6d31f2c8731 probably indicates that you're > right: > > if (dr && !dr->restore_intx) { > dr->restore_intx = 1; > dr->orig_intx = !enable; > } > > > So they used a boolean to only take the first state. Although that > still wouldn't have necessarily been the pre-driver INTx state. IIRC, this code path is reached only after checking that the INTx state is changed. Hence "!enable" is assured to be the pre-driver INTx state in the old code. > > > > 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); > > sth like: > > /* > * pcim_intx() must only restore the INTx value that existed before the > * driver was loaded, i.e., before it called pcim_intx() for the > * first time. > */ OK, will add it. > > + 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); > > Looks like a good idea to me > > The only thing I'm wondering about right now is the following: In the > old days, there was only pci_intx(), which either did devres or didn't. > > Now you have two functions, pcim_intx() and pci_intx(). > > The thing is that the driver could theoretically still intermingle them > and for example call pci_intx() before pcim_intx(), which would lead > the latter to still restore the wrong value. > > But that's very unlikely and I'm not sure whether we can do something > about it. Right, pcim_intx() assures to restore INTx value back to the moment it was called. And that should be enough and consistent behavior. BTW, a possible optimization would be to skip the devres if the value isn't really changed from the current state (which is similar like the old code before pcim_intx()). OTOH, this can lead to inconsistencies when INTx is changed manually after pcim_intx() somehow. So maybe it's not worth. thanks, Takashi
On Fri, 2024-10-25 at 16:52 +0200, Takashi Iwai wrote: > On Fri, 25 Oct 2024 16:28:42 +0200, > Philipp Stanner wrote: > > > > On Fri, 2024-10-25 at 12:44 +0200, Takashi Iwai wrote: > > > 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. btw following our discussion I think the commit message should then also explicitly state that pcim_intx() is supposed to restore the value before the function itself had been called by that driver for the very first time. > > > > > > > > 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. > > > > Alright, so you think that pcim_intx() should always restore the > > INTx > > state that existed before the driver was loaded. > > > > > > > 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@vger.kernel.org # 6.11+ > > > > > > OK. > > > > > > > > Link: https://lore.kernel.org/87v7xk2ps5.wl-tiwai@suse.de > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > > > --- > > > > > 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 might be the best solution. If it's done that way it should > > include a comment detailing the problem. > > > > Looking at the implementation of pci_intx() before > > 25216afc9db53d85dc648aba8fb7f6d31f2c8731 probably indicates that > > you're > > right: > > > > if (dr && !dr->restore_intx) { > > dr->restore_intx = 1; > > dr->orig_intx = !enable; > > } > > > > > > So they used a boolean to only take the first state. Although that > > still wouldn't have necessarily been the pre-driver INTx state. > > IIRC, this code path is reached only after checking that the INTx > state is changed. Hence "!enable" is assured to be the pre-driver > INTx state in the old code. Oh man. Have those folks never heard of comments :( > > > > > > > > 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); > > > > sth like: > > > > /* > > * pcim_intx() must only restore the INTx value that existed before > > the > > * driver was loaded, i.e., before it called pcim_intx() for the > > * first time. > > */ > > OK, will add it. > > > > + 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); > > > > Looks like a good idea to me > > > > The only thing I'm wondering about right now is the following: In > > the > > old days, there was only pci_intx(), which either did devres or > > didn't. > > > > Now you have two functions, pcim_intx() and pci_intx(). > > > > The thing is that the driver could theoretically still intermingle > > them > > and for example call pci_intx() before pcim_intx(), which would > > lead > > the latter to still restore the wrong value. > > > > But that's very unlikely and I'm not sure whether we can do > > something > > about it. > > Right, pcim_intx() assures to restore INTx value back to the moment > it > was called. And that should be enough and consistent behavior. The moment *before* it was called *for the first time*. > > BTW, a possible optimization would be to skip the devres if the value > isn't really changed from the current state (which is similar like > the > old code before pcim_intx()). OTOH, this can lead to inconsistencies > when INTx is changed manually after pcim_intx() somehow. So maybe > it's not worth. There is only a very tiny number of pci{m}_intx() users, they mostly use it in their probe() function, and future use is discouraged in favor of pci_alloc_irq_vectors(). So I think you can save yourself that effort. P. > > > thanks, > > Takashi >
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); 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;
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. 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. 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()") Link: https://lore.kernel.org/87v7xk2ps5.wl-tiwai@suse.de Signed-off-by: Takashi Iwai <tiwai@suse.de> --- drivers/pci/devres.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)