usb: yurex: Rearrange code not to need GFP_ATOMIC

PATCH In review Pavel Machek 3 participants 5 messages Sep 20, 2020 – Sep 21, 2020 USB

lore

Share & tools

lore

Pavel Machek

author

2020-09-20 08:45 UTC #0 lore raw

PATCH 1/1 usb: yurex: Rearrange code not to need GFP_ATOMIC

Move prepare to wait around, so that normal GFP_KERNEL allocation can be used.

Signed-off-by Pavel Machek (CIP) <[email protected]>

Acked-by Alan Stern <[email protected]>

drivers/usb/misc/yurex.c +2 -2

index b2e09883c7e2..071f1debebba 100644
@@ -489,10 +489,10 @@ static ssize_t yurex_write(struct file *file, const char __user *user_buffer,
 	}
 
 	/* send the data as the control msg */
-	prepare_to_wait(&dev->waitq, &wait, TASK_INTERRUPTIBLE);
 	dev_dbg(&dev->interface->dev, "%s - submit %c\n", __func__,
 		dev->cntl_buffer[0]);
-	retval = usb_submit_urb(dev->cntl_urb, GFP_ATOMIC);
+	retval = usb_submit_urb(dev->cntl_urb, GFP_KERNEL);
+	prepare_to_wait(&dev->waitq, &wait, TASK_INTERRUPTIBLE);
 	if (retval >= 0)
 		timeout = schedule_timeout(YUREX_WRITE_TIMEOUT);
 	finish_wait(&dev->waitq, &wait);

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Oliver Neukum

2020-09-21 12:25 UTC #1 lore raw

Am Sonntag, den 20.09.2020, 10:44 +0200 schrieb Pavel Machek:

Quoted 5 lines

Move prepare to wait around, so that normal GFP_KERNEL allocation can be used.
Signed-off-by Pavel Machek (CIP) <[email protected]>
Acked-by Alan Stern <[email protected]>

Ehm. Please recheck.

drivers/usb/misc/yurex.c static ssize_t yurex_write(struct file *file, const char __user *user_buffer, · line 494

Quoted 13 lines

drivers/usb/misc/yurex.c +1 -2

index b2e09883c7e2..071f1debebba 100644
@@ -489,10 +489,10 @@ static ssize_t yurex_write(struct file *file, const char __user *user_buffer,
 	}
 
 	/* send the data as the control msg */
-	prepare_to_wait(&dev->waitq, &wait, TASK_INTERRUPTIBLE);
 	dev_dbg(&dev->interface->dev, "%s - submit %c\n", __func__,
 		dev->cntl_buffer[0]);
-	retval = usb_submit_urb(dev->cntl_urb, GFP_ATOMIC);
+	retval = usb_submit_urb(dev->cntl_urb, GFP_KERNEL);

URB completes here. wake_up() returns the task to RUNNING.

drivers/usb/misc/yurex.c static ssize_t yurex_write(struct file *file, const char __user *user_buffer, · line 495

Quoted 1 line

drivers/usb/misc/yurex.c +1 -0

+	prepare_to_wait(&dev->waitq, &wait, TASK_INTERRUPTIBLE);

Task goes to TASK_INTERRUPTIBLE

drivers/usb/misc/yurex.c static ssize_t yurex_write(struct file *file, const char __user *user_buffer, · line 497

Quoted 2 lines

drivers/usb/misc/yurex.c +0 -0

 	if (retval >= 0)
 		timeout = schedule_timeout(YUREX_WRITE_TIMEOUT);

Task turns into Sleeping Beauty until timeout

drivers/usb/misc/yurex.c static ssize_t yurex_write(struct file *file, const char __user *user_buffer, · line 498

Quoted 1 line

drivers/usb/misc/yurex.c +0 -0
 	finish_wait(&dev->waitq, &wait);

And here task goes into error reporting as it checks timeout.

Regards
Oliver

Pavel Machek

author

2020-09-21 12:52 UTC #2 lore raw

Hi!

Quoted 7 lines

Quoted 5 lines
Move prepare to wait around, so that normal GFP_KERNEL allocation can be used.
Signed-off-by Pavel Machek (CIP) <[email protected]>
Acked-by Alan Stern <[email protected]>
Ehm. Please recheck.

Sorry about that.

Quoted 13 lines

drivers/usb/misc/yurex.c static ssize_t yurex_write(struct file *file, const char __user *user_buffer, · line 494
Quoted 10 lines
+++ b/drivers/usb/misc/yurex.c
drivers/usb/misc/yurex.c +1 -2
@@ -489,10 +489,10 @@ static ssize_t yurex_write(struct file *file, const char __user *user_buffer,
 	}
 
 	/* send the data as the control msg */
-	prepare_to_wait(&dev->waitq, &wait, TASK_INTERRUPTIBLE);
 	dev_dbg(&dev->interface->dev, "%s - submit %c\n", __func__,
 		dev->cntl_buffer[0]);
-	retval = usb_submit_urb(dev->cntl_urb, GFP_ATOMIC);
+	retval = usb_submit_urb(dev->cntl_urb, GFP_KERNEL);
URB completes here. wake_up() returns the task to RUNNING.

Quoted 8 lines

drivers/usb/misc/yurex.c static ssize_t yurex_write(struct file *file, const char __user *user_buffer, · line 495
Quoted 1 line
drivers/usb/misc/yurex.c +1 -0
+	prepare_to_wait(&dev->waitq, &wait, TASK_INTERRUPTIBLE);
Task goes to TASK_INTERRUPTIBLE
drivers/usb/misc/yurex.c static ssize_t yurex_write(struct file *file, const char __user *user_buffer, · line 497
Quoted 2 lines
drivers/usb/misc/yurex.c +0 -0
 	if (retval >= 0)
 		timeout = schedule_timeout(YUREX_WRITE_TIMEOUT);
Task turns into Sleeping Beauty until timeout

Is there way to do the allocations for submit_urb before the prepare_to_wait? GFP_ATOMIC would be nice to avoid... and doing GFP_ATOMIC from normal process context just because of task_state seems ... wrong.

Pavel

Signature · 2 lines

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Oliver Neukum

2020-09-21 13:00 UTC #3 lore raw

Am Montag, den 21.09.2020, 14:52 +0200 schrieb Pavel Machek:

Quoted 11 lines

Hi!
Quoted 7 lines
Task goes to TASK_INTERRUPTIBLE
drivers/usb/misc/yurex.c static ssize_t yurex_write(struct file *file, const char __user *user_buffer, · line 497
Quoted 2 lines
drivers/usb/misc/yurex.c +0 -0
 	if (retval >= 0)
 		timeout = schedule_timeout(YUREX_WRITE_TIMEOUT);
Task turns into Sleeping Beauty until timeout
Is there way to do the allocations for submit_urb before the

No. In theory you do not even know which HC will get the URB. Preallocating resources is impossible. I do consider this a design bug in the usbcore API.

Quoted 3 lines

prepare_to_wait? GFP_ATOMIC would be nice to avoid... and doing GFP_ATOMIC from normal process context just because of task_state seems ... wrong.

Well, then you will need to change the rest of the logic and use a struct completion. Give the age and practical relevance of the driver I would recommend against making such drastic changes and let it just be in its awkward but correct state.

Regards
Oliver

Alan Stern

2020-09-21 14:45 UTC #4 lore raw

In reply to Oliver Neukum

Quoted 29 lines · Oliver Neukum

Am Sonntag, den 20.09.2020, 10:44 +0200 schrieb Pavel Machek:
Quoted 5 lines
Move prepare to wait around, so that normal GFP_KERNEL allocation can be used.
Signed-off-by Pavel Machek (CIP) <[email protected]>
Acked-by Alan Stern <[email protected]>
Ehm. Please recheck.
drivers/usb/misc/yurex.c static ssize_t yurex_write(struct file *file, const char __user *user_buffer, · line 494
Quoted 13 lines
drivers/usb/misc/yurex.c +1 -2
index b2e09883c7e2..071f1debebba 100644
@@ -489,10 +489,10 @@ static ssize_t yurex_write(struct file *file, const char __user *user_buffer,
 	}
 
 	/* send the data as the control msg */
-	prepare_to_wait(&dev->waitq, &wait, TASK_INTERRUPTIBLE);
 	dev_dbg(&dev->interface->dev, "%s - submit %c\n", __func__,
 		dev->cntl_buffer[0]);
-	retval = usb_submit_urb(dev->cntl_urb, GFP_ATOMIC);
+	retval = usb_submit_urb(dev->cntl_urb, GFP_KERNEL);
URB completes here. wake_up() returns the task to RUNNING.
drivers/usb/misc/yurex.c static ssize_t yurex_write(struct file *file, const char __user *user_buffer, · line 495
Quoted 1 line
drivers/usb/misc/yurex.c +1 -0
+	prepare_to_wait(&dev->waitq, &wait, TASK_INTERRUPTIBLE);
Task goes to TASK_INTERRUPTIBLE

Quoted 8 lines

drivers/usb/misc/yurex.c static ssize_t yurex_write(struct file *file, const char __user *user_buffer, · line 497
Quoted 2 lines
drivers/usb/misc/yurex.c +0 -0
 	if (retval >= 0)
 		timeout = schedule_timeout(YUREX_WRITE_TIMEOUT);
Task turns into Sleeping Beauty until timeout
drivers/usb/misc/yurex.c static ssize_t yurex_write(struct file *file, const char __user *user_buffer, · line 498
Quoted 1 line
drivers/usb/misc/yurex.c +0 -0
 	finish_wait(&dev->waitq, &wait);
And here task goes into error reporting as it checks timeout.

Arrgh. Now I feel stupid. (Me and Pavel both, probably.)

Thanks for spotting this.

Alan Stern