| Message ID | 20240627021314.2976443-1-make24@iscas.ac.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] s390/ism: Add check for dma_set_max_seg_size in ism_probe() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 2024-06-27 at 07:43:14, Ma Ke (make24@iscas.ac.cn) wrote: > As the possible failure of the dma_set_max_seg_size(), we should better Could you expand on the scenario of failure ? > check the return value of the dma_set_max_seg_size(). > +++ b/drivers/s390/net/ism_drv.c > @@ -620,7 +620,10 @@ 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); Same error check is not valid for dma_set_seg_boundary() ? >
> 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,10 @@ 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) > + goto err_resource; > + > pci_set_master(pdev); … A) 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 B) 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
> B) 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 Hi Markus Please stop this. We have said a number of times, we don't want them in existing code, at least not yet. Please come back in a couple of years time once we know a bit more about how this helps/hinders. Andrew
diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c index e36e3ea165d3..54f6638e889c 100644 --- a/drivers/s390/net/ism_drv.c +++ b/drivers/s390/net/ism_drv.c @@ -620,7 +620,10 @@ 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) + goto err_resource; + pci_set_master(pdev); ret = ism_dev_init(ism);
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: b0da3498c587 ("PCI: Remove pci_set_dma_max_seg_size()") Signed-off-by: Ma Ke <make24@iscas.ac.cn> --- Changes in v2: - modified the patch according to suggestions; - modified Fixes line according to suggestions. --- drivers/s390/net/ism_drv.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)