diff mbox series

[PATCHv2] usb: hso: obey DMA rules in tiocmget

Message ID 20191017132548.21888-1-oneukum@suse.com
State Accepted
Delegated to: David Miller
Headers show
Series [PATCHv2] usb: hso: obey DMA rules in tiocmget | expand

Commit Message

Oliver Neukum Oct. 17, 2019, 1:25 p.m. UTC 
The serial state information must not be embedded into another
data structure, as this interferes with cache handling for DMA
on architectures without cache coherence..
That would result in data corruption on some architectures
Allocating it separately.

v2: fix syntax error

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/net/usb/hso.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Neil Jerram Oct. 17, 2019, 2:50 p.m. UTC | #1 
Oliver Neukum <oneukum@suse.com> writes:

> The serial state information must not be embedded into another
> data structure, as this interferes with cache handling for DMA
> on architectures without cache coherence..
> That would result in data corruption on some architectures

Could you say more what you mean by "some architectures"?  I wonder if
this is responsible for long-standing flakiness dealing with the HSO
modem in the GTA04 phone?

Best wishes,
   Neil
David Miller Oct. 17, 2019, 5:24 p.m. UTC | #2 
From: Neil Jerram <neiljerram@cantab.net>
Date: Thu, 17 Oct 2019 15:50:31 +0100

> Oliver Neukum <oneukum@suse.com> writes:
> 
>> The serial state information must not be embedded into another
>> data structure, as this interferes with cache handling for DMA
>> on architectures without cache coherence..
>> That would result in data corruption on some architectures
> 
> Could you say more what you mean by "some architectures"?  I wonder if
> this is responsible for long-standing flakiness dealing with the HSO
> modem in the GTA04 phone?

Some MIPS, older sparc, etc.

I think this is documented in DMA API usage documentation under Documentation/
even.
David Miller Oct. 17, 2019, 7:05 p.m. UTC | #3 
From: Oliver Neukum <oneukum@suse.com>
Date: Thu, 17 Oct 2019 15:25:47 +0200

> The serial state information must not be embedded into another
> data structure, as this interferes with cache handling for DMA
> on architectures without cache coherence..
> That would result in data corruption on some architectures
> Allocating it separately.
> 
> v2: fix syntax error
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>

Applied.... still disappointed that this respin even needed to
happen.
diff mbox series

Patch

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index a505b2ab88b8..74849da031fa 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -186,7 +186,7 @@  struct hso_tiocmget {
 	int    intr_completed;
 	struct usb_endpoint_descriptor *endp;
 	struct urb *urb;
-	struct hso_serial_state_notification serial_state_notification;
+	struct hso_serial_state_notification *serial_state_notification;
 	u16    prev_UART_state_bitmap;
 	struct uart_icount icount;
 };
@@ -1432,7 +1432,7 @@  static int tiocmget_submit_urb(struct hso_serial *serial,
 			 usb_rcvintpipe(usb,
 					tiocmget->endp->
 					bEndpointAddress & 0x7F),
-			 &tiocmget->serial_state_notification,
+			 tiocmget->serial_state_notification,
 			 sizeof(struct hso_serial_state_notification),
 			 tiocmget_intr_callback, serial,
 			 tiocmget->endp->bInterval);
@@ -1479,7 +1479,7 @@  static void tiocmget_intr_callback(struct urb *urb)
 	/* wIndex should be the USB interface number of the port to which the
 	 * notification applies, which should always be the Modem port.
 	 */
-	serial_state_notification = &tiocmget->serial_state_notification;
+	serial_state_notification = tiocmget->serial_state_notification;
 	if (serial_state_notification->bmRequestType != BM_REQUEST_TYPE ||
 	    serial_state_notification->bNotification != B_NOTIFICATION ||
 	    le16_to_cpu(serial_state_notification->wValue) != W_VALUE ||
@@ -2565,6 +2565,8 @@  static void hso_free_tiomget(struct hso_serial *serial)
 		usb_free_urb(tiocmget->urb);
 		tiocmget->urb = NULL;
 		serial->tiocmget = NULL;
+		kfree(tiocmget->serial_state_notification);
+		tiocmget->serial_state_notification = NULL;
 		kfree(tiocmget);
 	}
 }
@@ -2615,10 +2617,13 @@  static struct hso_device *hso_create_bulk_serial_device(
 		num_urbs = 2;
 		serial->tiocmget = kzalloc(sizeof(struct hso_tiocmget),
 					   GFP_KERNEL);
+		serial->tiocmget->serial_state_notification
+			= kzalloc(sizeof(struct hso_serial_state_notification),
+					   GFP_KERNEL);
 		/* it isn't going to break our heart if serial->tiocmget
 		 *  allocation fails don't bother checking this.
 		 */
-		if (serial->tiocmget) {
+		if (serial->tiocmget && serial->tiocmget->serial_state_notification) {
 			tiocmget = serial->tiocmget;
 			tiocmget->endp = hso_get_ep(interface,
 						    USB_ENDPOINT_XFER_INT,