LinuxLists.cc - [PATCH] drivers/usb: use READ_ONCE instead of deprecated ACCESS

2016-11-19 19:54:36

Subject: [PATCH] drivers/usb: use READ_ONCE instead of deprecated ACCESS_ONCE

With the new standardized functions, we can replace all ACCESS_ONCE()
calls across relevant drivers/usb/.

ACCESS_ONCE() does not work reliably on non-scalar types. For example
gcc 4.6 and 4.7 might remove the volatile tag for such accesses during
the SRA (scalar replacement of aggregates) step:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145

Update the new calls regardless of if it is a scalar type, this is
cleaner than having three alternatives.

Signed-off-by: Davidlohr Bueso <[email protected]>
---
drivers/usb/class/cdc-wdm.c | 2 +-
drivers/usb/core/devio.c | 2 +-
drivers/usb/core/sysfs.c | 4 ++--
drivers/usb/gadget/udc/gr_udc.c | 4 ++--
drivers/usb/host/ohci-hcd.c | 2 +-
drivers/usb/host/uhci-hcd.h | 4 ++--
6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 0a6369510f2d..d31ae7d1c2ac 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -515,7 +515,7 @@ static ssize_t wdm_read
if (rv < 0)
return -ERESTARTSYS;

- cntr = ACCESS_ONCE(desc->length);
+ cntr = READ_ONCE(desc->length);
if (cntr == 0) {
desc->read = 0;
retry:
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 4016dae7433b..ffd4389fb2e2 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -153,7 +153,7 @@ static int usbfs_increase_memory_usage(unsigned amount)
* Convert usbfs_memory_mb to bytes, avoiding overflows.
* 0 means use the hard limit (effectively unlimited).
*/
- lim = ACCESS_ONCE(usbfs_memory_mb);
+ lim = READ_ONCE(usbfs_memory_mb);
if (lim == 0 || lim > (USBFS_XFER_MAX >> 20))
lim = USBFS_XFER_MAX;
else
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index c953a0f1c695..048eece1884f 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -956,7 +956,7 @@ static ssize_t interface_show(struct device *dev, struct device_attribute *attr,
char *string;

intf = to_usb_interface(dev);
- string = ACCESS_ONCE(intf->cur_altsetting->string);
+ string = READ_ONCE(intf->cur_altsetting->string);
if (!string)
return 0;
return sprintf(buf, "%s\n", string);
@@ -972,7 +972,7 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,

intf = to_usb_interface(dev);
udev = interface_to_usbdev(intf);
- alt = ACCESS_ONCE(intf->cur_altsetting);
+ alt = READ_ONCE(intf->cur_altsetting);

return sprintf(buf, "usb:v%04Xp%04Xd%04Xdc%02Xdsc%02Xdp%02X"
"ic%02Xisc%02Xip%02Xin%02X\n",
diff --git a/drivers/usb/gadget/udc/gr_udc.c b/drivers/usb/gadget/udc/gr_udc.c
index 39b7136d31d9..2af6e8a98224 100644
--- a/drivers/usb/gadget/udc/gr_udc.c
+++ b/drivers/usb/gadget/udc/gr_udc.c
@@ -1261,7 +1261,7 @@ static int gr_handle_in_ep(struct gr_ep *ep)
if (!req->last_desc)
return 0;

- if (ACCESS_ONCE(req->last_desc->ctrl) & GR_DESC_IN_CTRL_EN)
+ if (READ_ONCE(req->last_desc->ctrl) & GR_DESC_IN_CTRL_EN)
return 0; /* Not put in hardware buffers yet */

if (gr_read32(&ep->regs->epstat) & (GR_EPSTAT_B1 | GR_EPSTAT_B0))
@@ -1290,7 +1290,7 @@ static int gr_handle_out_ep(struct gr_ep *ep)
if (!req->curr_desc)
return 0;

- ctrl = ACCESS_ONCE(req->curr_desc->ctrl);
+ ctrl = READ_ONCE(req->curr_desc->ctrl);
if (ctrl & GR_DESC_OUT_CTRL_EN)
return 0; /* Not received yet */

diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 86612ac3fda2..cf9ac6e3d157 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -784,7 +784,7 @@ static void io_watchdog_func(unsigned long _ohci)
}

/* find the last TD processed by the controller. */
- head = hc32_to_cpu(ohci, ACCESS_ONCE(ed->hwHeadP)) & TD_MASK;
+ head = hc32_to_cpu(ohci, READ_ONCE(ed->hwHeadP)) & TD_MASK;
td_start = td;
td_next = list_prepare_entry(td, &ed->td_list, td_list);
list_for_each_entry_continue(td_next, &ed->td_list, td_list) {
diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
index 6f986d82472d..033a2b46a46e 100644
--- a/drivers/usb/host/uhci-hcd.h
+++ b/drivers/usb/host/uhci-hcd.h
@@ -184,7 +184,7 @@ struct uhci_qh {
* We need a special accessor for the element pointer because it is
* subject to asynchronous updates by the controller.
*/
-#define qh_element(qh) ACCESS_ONCE((qh)->element)
+#define qh_element(qh) READ_ONCE((qh)->element)

#define LINK_TO_QH(uhci, qh) (UHCI_PTR_QH((uhci)) | \
cpu_to_hc32((uhci), (qh)->dma_handle))
@@ -272,7 +272,7 @@ struct uhci_td {
* subject to asynchronous updates by the controller.
*/
#define td_status(uhci, td) hc32_to_cpu((uhci), \
- ACCESS_ONCE((td)->status))
+ READ_ONCE((td)->status))

#define LINK_TO_TD(uhci, td) (cpu_to_hc32((uhci), (td)->dma_handle))

--
2.6.6

2016-11-20 09:39:24

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH] drivers/usb: use READ_ONCE instead of deprecated ACCESS_ONCE

On Sat, Nov 19, 2016 at 11:54:25AM -0800, Davidlohr Bueso wrote:
> With the new standardized functions, we can replace all ACCESS_ONCE()
> calls across relevant drivers/usb/.
>
> ACCESS_ONCE() does not work reliably on non-scalar types. For example
> gcc 4.6 and 4.7 might remove the volatile tag for such accesses during
> the SRA (scalar replacement of aggregates) step:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145
>
> Update the new calls regardless of if it is a scalar type, this is
> cleaner than having three alternatives.
>
> Signed-off-by: Davidlohr Bueso <[email protected]>

Nit, this doesn't match your From: line :(

If this is the case, why not just replacing the define for ACCESS_ONCE()
with READ_ONCE() and then go back and just do a search/replace for the
whole kernel all at once?

Or just send Linus a patch for this all at once after -rc1 is out?

thanks,

greg k-h

2016-11-20 12:11:53

by Mark Rutland

[permalink] [raw]

Subject: Re: [PATCH] drivers/usb: use READ_ONCE instead of deprecated ACCESS_ONCE

On Sun, Nov 20, 2016 at 10:39:32AM +0100, Greg KH wrote:
> On Sat, Nov 19, 2016 at 11:54:25AM -0800, Davidlohr Bueso wrote:
> > With the new standardized functions, we can replace all ACCESS_ONCE()
> > calls across relevant drivers/usb/.
> >
> > ACCESS_ONCE() does not work reliably on non-scalar types. For example
> > gcc 4.6 and 4.7 might remove the volatile tag for such accesses during
> > the SRA (scalar replacement of aggregates) step:

> If this is the case, why not just replacing the define for ACCESS_ONCE()
> with READ_ONCE() and then go back and just do a search/replace for the
> whole kernel all at once?
>
> Or just send Linus a patch for this all at once after -rc1 is out?

FWIW, I have a Coccinelle-generated patch for that [1].

... there are a few cases where it doesn't quite work (e.g. formatting in the
ath9k mac drivers), and I'm trying to put together a series [2] that fixes
those cases up before leaving the bulk to Coccinelle.

I hope to sort that out in the next few days.

Thanks,
Mark.

[1] https://git.kernel.org/cgit/linux/kernel/git/mark/linux.git/commit/?h=core/access-once&id=7154d3cd5f2ddcc029d1afadc472c5720408d678
[2] https://git.kernel.org/cgit/linux/kernel/git/mark/linux.git/log/?h=core/access-once

2016-11-20 16:11:03

by Davidlohr Bueso

[permalink] [raw]

Subject: Re: [PATCH] drivers/usb: use READ_ONCE instead of deprecated ACCESS_ONCE

Hi Greg!

On Sun, 20 Nov 2016, Greg KH wrote:

>On Sat, Nov 19, 2016 at 11:54:25AM -0800, Davidlohr Bueso wrote:
>> With the new standardized functions, we can replace all ACCESS_ONCE()
>> calls across relevant drivers/usb/.
>>
>> ACCESS_ONCE() does not work reliably on non-scalar types. For example
>> gcc 4.6 and 4.7 might remove the volatile tag for such accesses during
>> the SRA (scalar replacement of aggregates) step:
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145
>>
>> Update the new calls regardless of if it is a scalar type, this is
>> cleaner than having three alternatives.
>>
>> Signed-off-by: Davidlohr Bueso <[email protected]>
>
>Nit, this doesn't match your From: line :(

That's on purpose, and all my patches are the same.

>If this is the case, why not just replacing the define for ACCESS_ONCE()
>with READ_ONCE() and then go back and just do a search/replace for the
>whole kernel all at once?

So that we don't have three variants; the idea is to eventually
get rid of ACCESS_ONCE entirely.

>
>Or just send Linus a patch for this all at once after -rc1 is out?

No rush, whenever you see fit.

Thanks,
Davidlohr

2016-11-21 07:15:38

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH] drivers/usb: use READ_ONCE instead of deprecated ACCESS_ONCE

On Sun, Nov 20, 2016 at 08:09:40AM -0800, Davidlohr Bueso wrote:
> Hi Greg!
>
> On Sun, 20 Nov 2016, Greg KH wrote:
>
> > On Sat, Nov 19, 2016 at 11:54:25AM -0800, Davidlohr Bueso wrote:
> > > With the new standardized functions, we can replace all ACCESS_ONCE()
> > > calls across relevant drivers/usb/.
> > >
> > > ACCESS_ONCE() does not work reliably on non-scalar types. For example
> > > gcc 4.6 and 4.7 might remove the volatile tag for such accesses during
> > > the SRA (scalar replacement of aggregates) step:
> > >
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145
> > >
> > > Update the new calls regardless of if it is a scalar type, this is
> > > cleaner than having three alternatives.
> > >
> > > Signed-off-by: Davidlohr Bueso <[email protected]>
> >
> > Nit, this doesn't match your From: line :(
>
> That's on purpose, and all my patches are the same.

So they are all incorrect? Not good, why? You know this means I can't
take them...

> > If this is the case, why not just replacing the define for ACCESS_ONCE()
> > with READ_ONCE() and then go back and just do a search/replace for the
> > whole kernel all at once?
>
> So that we don't have three variants; the idea is to eventually
> get rid of ACCESS_ONCE entirely.

Then just get rid of it all at once.

thanks,

greg k-h

2016-11-29 19:24:26

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH] drivers/usb: use READ_ONCE instead of deprecated ACCESS_ONCE

2016-11-30 13:51:13

by Davidlohr Bueso

[permalink] [raw]

Subject: [PATCH v2] drivers/usb: use READ_ONCE instead of deprecated ACCESS_ONCE

2016-11-30 14:23:08

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH v2] drivers/usb: use READ_ONCE instead of deprecated ACCESS_ONCE

On Wed, Nov 30, 2016 at 05:50:54AM -0800, Davidlohr Bueso wrote:
> With the new standardized functions, we can replace all ACCESS_ONCE()
> calls across relevant drivers/usb/.
>
> ACCESS_ONCE() does not work reliably on non-scalar types. For example
> gcc 4.6 and 4.7 might remove the volatile tag for such accesses during
> the SRA (scalar replacement of aggregates) step:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145
>
> Update the new calls regardless of if it is a scalar type, this is
> cleaner than having three alternatives.
>
> Signed-off-by: Davidlohr Bueso <[email protected]>
> ---
> drivers/usb/class/cdc-wdm.c | 2 +-
> drivers/usb/core/devio.c | 2 +-
> drivers/usb/core/sysfs.c | 4 ++--
> drivers/usb/gadget/udc/gr_udc.c | 4 ++--
> drivers/usb/host/ohci-hcd.c | 2 +-
> drivers/usb/host/uhci-hcd.h | 4 ++--
> 6 files changed, 9 insertions(+), 9 deletions(-)

Same comment on your driver base patch, what changed from v1?