[PATCH 1/1] s390/virtio_ccw: fix dma_parm pointer not set up

[PATCH 1/1] s390/virtio_ccw: fix dma_parm pointer not set up

[Halil Pasic]

At least since commit 334304ac2bac ("dma-mapping: don't return errors
from dma_set_max_seg_size") setting up device.dma_parms is basically
mandated by the DMA API. As of now Channel (CCW) I/O in general does not
utilize the DMA API, except for virtio. For virtio-ccw however the
common virtio DMA infrastructure is such that most of the DMA stuff
hinges on the virtio parent device, which is a CCW device.

So lets set up the dma_parms pointer for the CCW parent device and hope
for the best!

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Fixes: 334304ac2bac ("dma-mapping: don't return errors from dma_set_max_seg_size")
Reported-by: "Marc Hartmayer" <mhartmay@linux.ibm.com>
Closes: https://bugzilla.linux.ibm.com/show_bug.cgi?id=209131
Reviewed-by: Eric Farman <farman@linux.ibm.com>
---

In the long run it may make sense to move dma_parms into struct
ccw_device, since layering-wise it is much cleaner. I decided
to put it in virtio_ccw_device because currently it is only used for
virtio.

---
 drivers/s390/virtio/virtio_ccw.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index 62eca9419ad7..21fa7ac849e5 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -58,6 +58,8 @@ struct virtio_ccw_device {
 	struct virtio_device vdev;
 	__u8 config[VIRTIO_CCW_CONFIG_SIZE];
 	struct ccw_device *cdev;
+	/* we make cdev->dev.dma_parms point to this */
+	struct device_dma_parameters dma_parms;
 	__u32 curr_io;
 	int err;
 	unsigned int revision; /* Transport revision */
@@ -1303,6 +1305,7 @@ static int virtio_ccw_offline(struct ccw_device *cdev)
 	unregister_virtio_device(&vcdev->vdev);
 	spin_lock_irqsave(get_ccwdev_lock(cdev), flags);
 	dev_set_drvdata(&cdev->dev, NULL);
+	cdev->dev.dma_parms = NULL;
 	spin_unlock_irqrestore(get_ccwdev_lock(cdev), flags);
 	return 0;
 }
@@ -1366,6 +1369,7 @@ static int virtio_ccw_online(struct ccw_device *cdev)
 	}
 	vcdev->vdev.dev.parent = &cdev->dev;
 	vcdev->cdev = cdev;
+	cdev->dev.dma_parms = &vcdev->dma_parms;
 	vcdev->dma_area = ccw_device_dma_zalloc(vcdev->cdev,
 						sizeof(*vcdev->dma_area),
 						&vcdev->dma_area_addr);

base-commit: 87d6aab2389e5ce0197d8257d5f8ee965a67c4cd
-- 
2.43.0

Re: [PATCH 1/1] s390/virtio_ccw: fix dma_parm pointer not set up

[Cornelia Huck]

On Mon, Oct 07 2024, Halil Pasic <pasic@linux.ibm.com> wrote:

> At least since commit 334304ac2bac ("dma-mapping: don't return errors
> from dma_set_max_seg_size") setting up device.dma_parms is basically
> mandated by the DMA API. As of now Channel (CCW) I/O in general does not
> utilize the DMA API, except for virtio. For virtio-ccw however the
> common virtio DMA infrastructure is such that most of the DMA stuff
> hinges on the virtio parent device, which is a CCW device.
>
> So lets set up the dma_parms pointer for the CCW parent device and hope
> for the best!
>
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Fixes: 334304ac2bac ("dma-mapping: don't return errors from dma_set_max_seg_size")
> Reported-by: "Marc Hartmayer" <mhartmay@linux.ibm.com>
> Closes: https://bugzilla.linux.ibm.com/show_bug.cgi?id=209131
> Reviewed-by: Eric Farman <farman@linux.ibm.com>
> ---
>
> In the long run it may make sense to move dma_parms into struct
> ccw_device, since layering-wise it is much cleaner. I decided
> to put it in virtio_ccw_device because currently it is only used for
> virtio.

Yes, ccw_device would make more sense as a resting place; no idea what
other devices (dasd, QDIO based, ...) would do with it ATM -- I agree
that if adding it to virtio_ccw_device get things going again, we should
do that and consider the possible generic case later.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[I assume this one can be picked up together with other s390 patches?]

>
> ---
>  drivers/s390/virtio/virtio_ccw.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index 62eca9419ad7..21fa7ac849e5 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -58,6 +58,8 @@ struct virtio_ccw_device {
>  	struct virtio_device vdev;
>  	__u8 config[VIRTIO_CCW_CONFIG_SIZE];
>  	struct ccw_device *cdev;
> +	/* we make cdev->dev.dma_parms point to this */
> +	struct device_dma_parameters dma_parms;
>  	__u32 curr_io;
>  	int err;
>  	unsigned int revision; /* Transport revision */
> @@ -1303,6 +1305,7 @@ static int virtio_ccw_offline(struct ccw_device *cdev)
>  	unregister_virtio_device(&vcdev->vdev);
>  	spin_lock_irqsave(get_ccwdev_lock(cdev), flags);
>  	dev_set_drvdata(&cdev->dev, NULL);
> +	cdev->dev.dma_parms = NULL;
>  	spin_unlock_irqrestore(get_ccwdev_lock(cdev), flags);
>  	return 0;
>  }
> @@ -1366,6 +1369,7 @@ static int virtio_ccw_online(struct ccw_device *cdev)
>  	}
>  	vcdev->vdev.dev.parent = &cdev->dev;
>  	vcdev->cdev = cdev;
> +	cdev->dev.dma_parms = &vcdev->dma_parms;
>  	vcdev->dma_area = ccw_device_dma_zalloc(vcdev->cdev,
>  						sizeof(*vcdev->dma_area),
>  						&vcdev->dma_area_addr);
>
> base-commit: 87d6aab2389e5ce0197d8257d5f8ee965a67c4cd

Re: [PATCH 1/1] s390/virtio_ccw: fix dma_parm pointer not set up

[Marc Hartmayer]

On Mon, Oct 07, 2024 at 10:10 PM +0200, Halil Pasic <pasic@linux.ibm.com> wrote:
> At least since commit 334304ac2bac ("dma-mapping: don't return errors
> from dma_set_max_seg_size") setting up device.dma_parms is basically
> mandated by the DMA API. As of now Channel (CCW) I/O in general does not
> utilize the DMA API, except for virtio. For virtio-ccw however the
> common virtio DMA infrastructure is such that most of the DMA stuff
> hinges on the virtio parent device, which is a CCW device.
>
> So lets set up the dma_parms pointer for the CCW parent device and hope
> for the best!
>
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Fixes: 334304ac2bac ("dma-mapping: don't return errors from dma_set_max_seg_size")
> Reported-by: "Marc Hartmayer" <mhartmay@linux.ibm.com>
> Closes: https://bugzilla.linux.ibm.com/show_bug.cgi?id=209131

I guess, this line can be removed as it’s internal only.

> Reviewed-by: Eric Farman <farman@linux.ibm.com>
> ---
>
> In the long run it may make sense to move dma_parms into struct
> ccw_device, since layering-wise it is much cleaner. I decided
> to put it in virtio_ccw_device because currently it is only used for
> virtio.
>
> ---

[…snip…]

Thanks for fixing this!

Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>

-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 1/1] s390/virtio_ccw: fix dma_parm pointer not set up

[Halil Pasic]

On Tue, 08 Oct 2024 10:47:48 +0200
"Marc Hartmayer" <mhartmay@linux.ibm.com> wrote:

> > Closes: https://bugzilla.linux.ibm.com/show_bug.cgi?id=209131  
> 
> I guess, this line can be removed as it’s internal only.

checkpatch.pl complains about the Reported-by if I do. 

It does not complain about
Closes: N/A
but if I read the process documentation correctly if the report
is not available on the web Closes should be omitted:
"""
Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:
----------------------------------------------------------------------

The Reported-by tag gives credit to people who find bugs and report them and it
hopefully inspires them to help us again in the future. The tag is intended for
bugs; please do not use it to credit feature requests. The tag should be
followed by a Closes: tag pointing to the report, unless the report is not
available on the web.
"""

So I guess I have to make peace with getting checkpatch warnings when I
give credits to the reporter for reports not available on the web.

Regards,
Halil

Re: [PATCH 1/1] s390/virtio_ccw: fix dma_parm pointer not set up

[Heiko Carstens]

On Mon, Oct 07, 2024 at 10:10:30PM +0200, Halil Pasic wrote:
> At least since commit 334304ac2bac ("dma-mapping: don't return errors
> from dma_set_max_seg_size") setting up device.dma_parms is basically
> mandated by the DMA API. As of now Channel (CCW) I/O in general does not
> utilize the DMA API, except for virtio. For virtio-ccw however the
> common virtio DMA infrastructure is such that most of the DMA stuff
> hinges on the virtio parent device, which is a CCW device.
> 
> So lets set up the dma_parms pointer for the CCW parent device and hope
> for the best!
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Fixes: 334304ac2bac ("dma-mapping: don't return errors from dma_set_max_seg_size")
> Reported-by: "Marc Hartmayer" <mhartmay@linux.ibm.com>
> Closes: https://bugzilla.linux.ibm.com/show_bug.cgi?id=209131
> Reviewed-by: Eric Farman <farman@linux.ibm.com>
> ---

Applied, thanks!