[PATCH] s390/ism: Add check for dma_set_max_seg_size in ism_probe()

[PATCH] s390/ism: Add check for dma_set_max_seg_size in ism_probe()

[Ma Ke]

As the possible failure of the dma_set_max_seg_size(), we should better
check the return value of the dma_set_max_seg_size().

Fixes: 684b89bc39ce ("s390/ism: add device driver for internal shared memory")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
 drivers/s390/net/ism_drv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
index e36e3ea165d3..9ddd093a0368 100644
--- a/drivers/s390/net/ism_drv.c
+++ b/drivers/s390/net/ism_drv.c
@@ -620,7 +620,9 @@ static int ism_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto err_resource;
 
 	dma_set_seg_boundary(&pdev->dev, SZ_1M - 1);
-	dma_set_max_seg_size(&pdev->dev, SZ_1M);
+	ret = dma_set_max_seg_size(&pdev->dev, SZ_1M);
+	if (ret)
+		return ret;
 	pci_set_master(pdev);
 
 	ret = ism_dev_init(ism);
-- 
2.25.1

Re: [PATCH] s390/ism: Add check for dma_set_max_seg_size in ism_probe()

[Gerd Bayer]

Hi Ma Ke,

On Wed, 2024-06-26 at 16:12 +0800, Ma Ke wrote:
> As the possible failure of the dma_set_max_seg_size(), we should
> better check the return value of the dma_set_max_seg_size().

I think formally you're correct. dma_set_max_seg_size() could return an
error if dev->dma_parms was not present.

However, since ISM devices are PCI attached (and will remain PCI
attached I believe) we can take the existance of dev->dma_parms for
granted since pci_device_add() (in drivers/pci/probe.c) will make that
point to the pci_dev's dma_parms for every PCI device.

So I'm not sure how important this fix is.

> Fixes: 684b89bc39ce ("s390/ism: add device driver for internal shared
> memory")
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> ---
>  drivers/s390/net/ism_drv.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
> index e36e3ea165d3..9ddd093a0368 100644
> --- a/drivers/s390/net/ism_drv.c
> +++ b/drivers/s390/net/ism_drv.c
> @@ -620,7 +620,9 @@ static int ism_probe(struct pci_dev *pdev, const
> struct pci_device_id *id)
>  		goto err_resource;
>  
>  	dma_set_seg_boundary(&pdev->dev, SZ_1M - 1);
> -	dma_set_max_seg_size(&pdev->dev, SZ_1M);
> +	ret = dma_set_max_seg_size(&pdev->dev, SZ_1M);
> +	if (ret)
> +		return ret;
>  	pci_set_master(pdev);
>  
>  	ret = ism_dev_init(ism);

BTW, I've dropped ubraun@linux.ibm.com and sebott@linux.ibm.com as
their emails won't work any longer, anyhow. Instead I've added Niklas
Schnelle, Wenjia Zhang and Stefan Raspl.

Thanks, Gerd

Re: [PATCH] s390/ism: Add check for dma_set_max_seg_size in ism_probe()

[Markus Elfring]

> > As the possible failure of the dma_set_max_seg_size(), we should
> > better check the return value of the dma_set_max_seg_size().
>
> I think formally you're correct. dma_set_max_seg_size() could return an
> error if dev->dma_parms was not present.
>
> However, since ISM devices are PCI attached (and will remain PCI
> attached I believe) we can take the existance of dev->dma_parms for
> granted since pci_device_add() (in drivers/pci/probe.c) will make that
> point to the pci_dev's dma_parms for every PCI device.
>
> So I'm not sure how important this fix is.

Another function call can fail eventually.

dma_set_seg_boundary()
https://elixir.bootlin.com/linux/v6.10-rc5/source/include/linux/dma-mapping.h#L562

Will it become relevant to complete the error detection
and corresponding exception handling any further?

Regards,
Markus

Re: [PATCH] s390/ism: Add check for dma_set_max_seg_size in ism_probe()

[Niklas Schnelle]

On Wed, 2024-06-26 at 14:48 +0200, Gerd Bayer wrote:
> Hi Ma Ke,
> 
> On Wed, 2024-06-26 at 16:12 +0800, Ma Ke wrote:
> > As the possible failure of the dma_set_max_seg_size(), we should
> > better check the return value of the dma_set_max_seg_size().
> 
> I think formally you're correct. dma_set_max_seg_size() could return an
> error if dev->dma_parms was not present.
> 
> However, since ISM devices are PCI attached (and will remain PCI
> attached I believe) we can take the existance of dev->dma_parms for
> granted since pci_device_add() (in drivers/pci/probe.c) will make that
> point to the pci_dev's dma_parms for every PCI device.
> 
> So I'm not sure how important this fix is.

I agree this doesn't look like an issue we're likely to encounter. That
said if for some reason dma_set_max_seg_size() or common PCI code is
changed and it starts failing it's good to have it handled.

This  line has also only been dma_set_max_seg_size() since commit
b0da3498c587 ("PCI: Remove pci_set_dma_max_seg_size()"). Since this
patch won't apply for anything older I would prefer to cite that as the
fixed commit.

> 
> > Fixes: 684b89bc39ce ("s390/ism: add device driver for internal shared
> > memory")
> > Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> > ---
> >  drivers/s390/net/ism_drv.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
> > index e36e3ea165d3..9ddd093a0368 100644
> > --- a/drivers/s390/net/ism_drv.c
> > +++ b/drivers/s390/net/ism_drv.c
> > @@ -620,7 +620,9 @@ static int ism_probe(struct pci_dev *pdev, const
> > struct pci_device_id *id)
> >  		goto err_resource;
                ^ see here

> >  
> >  	dma_set_seg_boundary(&pdev->dev, SZ_1M - 1);
> > -	dma_set_max_seg_size(&pdev->dev, SZ_1M);
> > +	ret = dma_set_max_seg_size(&pdev->dev, SZ_1M);
> > +	if (ret)
> > +		return ret;

Simply returning here would leak the resources. Just like the error
condition I marked above this would need to be goto err_resource.

> >  	pci_set_master(pdev);
> >  
> >  	ret = ism_dev_init(ism);
> 
> BTW, I've dropped ubraun@linux.ibm.com and sebott@linux.ibm.com as
> their emails won't work any longer, anyhow. Instead I've added Niklas
> Schnelle, Wenjia Zhang and Stefan Raspl.
> 
> Thanks, Gerd
> 

Re: [PATCH] s390/ism: Add check for dma_set_max_seg_size in ism_probe()

[Christoph Hellwig]

On Wed, Jun 26, 2024 at 02:48:30PM +0200, Gerd Bayer wrote:
> 
> However, since ISM devices are PCI attached (and will remain PCI
> attached I believe) we can take the existance of dev->dma_parms for
> granted since pci_device_add() (in drivers/pci/probe.c) will make that
> point to the pci_dev's dma_parms for every PCI device.
> 
> So I'm not sure how important this fix is.

It's not just important, it is stupid and I told them to stop sending
these kinds of crap patches.

Re: [PATCH] s390/ism: Add check for dma_set_max_seg_size in ism_probe()

[Markus Elfring]

> As the possible failure of the dma_set_max_seg_size(), we should better
> check the return value of the dma_set_max_seg_size().

Please avoid the repetition of a function name in such a change description.
Can it be improved with corresponding imperative wordings?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc5#n94


…
> +++ b/drivers/s390/net/ism_drv.c
> @@ -620,7 +620,9 @@ static int ism_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		goto err_resource;
>
>  	dma_set_seg_boundary(&pdev->dev, SZ_1M - 1);
> -	dma_set_max_seg_size(&pdev->dev, SZ_1M);
> +	ret = dma_set_max_seg_size(&pdev->dev, SZ_1M);
> +	if (ret)
> +		return ret;
>  	pci_set_master(pdev);
…

1. Will the shown dma_set_seg_boundary() call trigger similar software development concerns?
   https://elixir.bootlin.com/linux/v6.10-rc5/source/include/linux/dma-mapping.h#L562


2. Would the following statement be more appropriate for the proposed completion
   of the exception handling?

+		goto err_resource;


3. Under which circumstances would you become interested to increase the application
   of scope-based resource management here?
   https://elixir.bootlin.com/linux/v6.10-rc5/source/include/linux/cleanup.h#L8


Regards,
Markus

Re: [PATCH] s390/ism: Add check for dma_set_max_seg_size in ism_probe()

[Christoph Hellwig]

On Wed, Jun 26, 2024 at 04:12:15PM +0800, Ma Ke wrote:
> As the possible failure of the dma_set_max_seg_size(), we should better
> check the return value of the dma_set_max_seg_size().

As I told you last time these checks are stupid, and I asked you to
instead send a patch to remove the return value.

Each and every of those patches just makes that removal harder.
Please stop this now and I'll prepare the removal for the next
merge window.