Re: [linux-usb-devel] USB on PPC440GP (cache incoherent)

Hi,

the basic idea is sound, but ...

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux 2.4 for PowerPC development tree
# This patch format is intended for GNU patch command version 2.5 or
higher. # This patch includes the following deltas:
#	           ChangeSet	1.1077  -> 1.1078
#	   drivers/usb/usb.c	1.19    -> 1.20
#	   drivers/usb/hub.c	1.14    -> 1.15
#	   drivers/usb/hub.h	1.7     -> 1.8
#	drivers/usb/storage/transport.c	1.10    -> 1.11
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/06/07	roland@gold.topspincom.com	1.1078
# Fix problems in USB when running on cache-incoherent cpus like PPC440GP
# (Don't do DMAs into memory allocated on stack)
# --------------------------------------------
#

This can be used in error handling by storage devices. You must use GFP_NOIO.
And you should check for a failure due to OOM.

+	ret = usb_get_descriptor(dev, USB_DT_DEVICE, 0, descriptor,
+                                 sizeof *descriptor);
 	if (ret < 0)
 		return ret;

-	le16_to_cpus(&descriptor.bcdUSB);
-	le16_to_cpus(&descriptor.idVendor);
-	le16_to_cpus(&descriptor.idProduct);
-	le16_to_cpus(&descriptor.bcdDevice);
+	le16_to_cpus(&descriptor->bcdUSB);
+	le16_to_cpus(&descriptor->idVendor);
+	le16_to_cpus(&descriptor->idProduct);
+	le16_to_cpus(&descriptor->bcdDevice);

-	if (memcmp(&dev->descriptor, &descriptor, sizeof(descriptor))) {
+	if (memcmp(&dev->descriptor, descriptor, sizeof *descriptor)) {
+                kfree(descriptor);
 		usb_destroy_configuration(dev);

 		ret = usb_get_device_descriptor(dev);

Must be "? GFP_ATOMIC : GFP_NOIO" or you could deadlock.
However, why do you do this ? The srb is kmalloced.

 		/* set the buffer length for transfer */
 		old_request_bufflen = srb->request_bufflen;

Here again you cannot use GFP_KERNEL. It must be GFP_NOIO instead.

+                kfree(bcb);
+                return USB_STOR_TRANSPORT_ERROR;
+        }

 	/* set up the command wrapper */
-	bcb.Signature = cpu_to_le32(US_BULK_CB_SIGN);
-	bcb.DataTransferLength = cpu_to_le32(usb_stor_transfer_length(srb));
-	bcb.Flags = srb->sc_data_direction == SCSI_DATA_READ ? 1 << 7 : 0;
-	bcb.Tag = srb->serial_number;
-	bcb.Lun = srb->cmnd[1] >> 5;
+	bcb->Signature = cpu_to_le32(US_BULK_CB_SIGN);
+	bcb->DataTransferLength = cpu_to_le32(usb_stor_transfer_length(srb));
+	bcb->Flags = srb->sc_data_direction == SCSI_DATA_READ ? 1 << 7 : 0;
+	bcb->Tag = srb->serial_number;
+	bcb->Lun = srb->cmnd[1] >> 5;
 	if (us->flags & US_FL_SCM_MULT_TARG)
-		bcb.Lun |= srb->target << 4;
-	bcb.Length = srb->cmd_len;
+		bcb->Lun |= srb->target << 4;
+	bcb->Length = srb->cmd_len;

 	/* construct the pipe handle */
 	pipe = usb_sndbulkpipe(us->pusb_dev, us->ep_out);

 	/* copy the command payload */
-	memset(bcb.CDB, 0, sizeof(bcb.CDB));
-	memcpy(bcb.CDB, srb->cmnd, bcb.Length);
+	memset(bcb->CDB, 0, sizeof(bcb->CDB));
+	memcpy(bcb->CDB, srb->cmnd, bcb->Length);

 	/* send it to out endpoint */
 	US_DEBUGP("Bulk command S 0x%x T 0x%x Trg %d LUN %d L %d F %d CL %d\n",
-		  le32_to_cpu(bcb.Signature), bcb.Tag,
-		  (bcb.Lun >> 4), (bcb.Lun & 0x0F),
-		  bcb.DataTransferLength, bcb.Flags, bcb.Length);
-	result = usb_stor_bulk_msg(us, &bcb, pipe, US_BULK_CB_WRAP_LEN,
+		  le32_to_cpu(bcb->Signature), bcb->Tag,
+		  (bcb->Lun >> 4), (bcb->Lun & 0x0F),
+		  bcb->DataTransferLength, bcb->Flags, bcb->Length);
+	result = usb_stor_bulk_msg(us, bcb, pipe, US_BULK_CB_WRAP_LEN,
 				   &partial);
 	US_DEBUGP("Bulk command transfer result=%d\n", result);

 	/* if the command was aborted, indicate that */
-	if (result == -ENOENT)
-		return USB_STOR_TRANSPORT_ABORTED;
+	if (result == -ENOENT) {
+                ret = USB_STOR_TRANSPORT_ABORTED;
+                goto out;
+        }

 	/* if we stall, we need to clear it before we go on */
 	if (result == -EPIPE) {

usb_control_msg is used. This cannot be called in interrupt.

+        if (!tmp_buf) {
+          return -ENOMEM;
+        }
+	memset(tmp_buf,0,size);	// Make sure we parse really received data

 	while (i--) {
 		if ((result = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
 			USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
-			(type << 8) + index, 0, buf, size, HZ * GET_TIMEOUT)) > 0 ||
+			(type << 8) + index, 0, tmp_buf, size, HZ * GET_TIMEOUT)) > 0 ||
 		     result == -EPIPE)
 			break;	/* retry if the returned length was 0; flaky device */
 	}
+        memcpy(buf, tmp_buf, size);
+        kfree(tmp_buf);
 	return result;
 }

	HTH
		Oliver

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/