| Message ID | 1560424728-13929-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | usb: renesas_usbhs: Use struct assignment instead of memcpy() | expand |
On Thu, Jun 13, 2019 at 08:18:48PM +0900, Yoshihiro Shimoda wrote: > To avoid the error-proneness of calls to sizeof() in the memcpy, > this patch uses struct assignment instead of memcpy. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
On Thu, Jun 13, 2019 at 08:18:48PM +0900, Yoshihiro Shimoda wrote: > To avoid the error-proneness of calls to sizeof() in the memcpy, > this patch uses struct assignment instead of memcpy. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > This patch is based on Greg's linux-usb.git / usb-next branch. > Note that mod_host.c also has memcpy but we cannot use struct assignment > for it because the type of urb->setup_patcket is just "unsigned char *". > > drivers/usb/renesas_usbhs/common.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c > index a501ea6..ebbe322 100644 > --- a/drivers/usb/renesas_usbhs/common.c > +++ b/drivers/usb/renesas_usbhs/common.c > @@ -651,9 +651,8 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev) > return NULL; > > dparam = &info->driver_param; > - memcpy(dparam, &data->param, sizeof(data->param)); > - memcpy(&info->platform_callback, data->platform_callback, > - sizeof(*data->platform_callback)); > + *dparam = data->param; > + info->platform_callback = *data->platform_callback; How are the original calls here "error-prone"? Yes, the compiler will end up calling memcpy somehow with this change, but it feels "wrong" to hide a memory copy like this. So are you _sure_ you want this change? thanks, greg k-h
Hi Greg, On Tue, Jun 18, 2019 at 9:17 AM Greg KH <gregkh@linuxfoundation.org> wrote: > On Thu, Jun 13, 2019 at 08:18:48PM +0900, Yoshihiro Shimoda wrote: > > To avoid the error-proneness of calls to sizeof() in the memcpy, > > this patch uses struct assignment instead of memcpy. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > This patch is based on Greg's linux-usb.git / usb-next branch. > > Note that mod_host.c also has memcpy but we cannot use struct assignment > > for it because the type of urb->setup_patcket is just "unsigned char *". > > > > drivers/usb/renesas_usbhs/common.c | 13 ++++--------- > > 1 file changed, 4 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c > > index a501ea6..ebbe322 100644 > > --- a/drivers/usb/renesas_usbhs/common.c > > +++ b/drivers/usb/renesas_usbhs/common.c > > @@ -651,9 +651,8 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev) > > return NULL; > > > > dparam = &info->driver_param; > > - memcpy(dparam, &data->param, sizeof(data->param)); > > - memcpy(&info->platform_callback, data->platform_callback, > > - sizeof(*data->platform_callback)); > > + *dparam = data->param; > > + info->platform_callback = *data->platform_callback; > > How are the original calls here "error-prone"? Yes, the compiler will > end up calling memcpy somehow with this change, but it feels "wrong" to > hide a memory copy like this. There are no checks that: - the source and destination pointers point to the same type, - the passed size matches the actual object size. Gr{oetje,eeting}s, Geert
Hi Greg, > From: Greg KH, Sent: Tuesday, June 18, 2019 2:59 PM > > On Thu, Jun 13, 2019 at 08:18:48PM +0900, Yoshihiro Shimoda wrote: > > To avoid the error-proneness of calls to sizeof() in the memcpy, > > this patch uses struct assignment instead of memcpy. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > This patch is based on Greg's linux-usb.git / usb-next branch. > > Note that mod_host.c also has memcpy but we cannot use struct assignment > > for it because the type of urb->setup_patcket is just "unsigned char *". > > > > drivers/usb/renesas_usbhs/common.c | 13 ++++--------- > > 1 file changed, 4 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c > > index a501ea6..ebbe322 100644 > > --- a/drivers/usb/renesas_usbhs/common.c > > +++ b/drivers/usb/renesas_usbhs/common.c > > @@ -651,9 +651,8 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev) > > return NULL; > > > > dparam = &info->driver_param; > > - memcpy(dparam, &data->param, sizeof(data->param)); > > - memcpy(&info->platform_callback, data->platform_callback, > > - sizeof(*data->platform_callback)); > > + *dparam = data->param; > > + info->platform_callback = *data->platform_callback; > > How are the original calls here "error-prone"? When I was developing the patch above [1], I had actually confused about these arguments of memcpy... [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/drivers/usb/renesas_usbhs?h=usb-next&id=be21a02a5a5ec88b12ce535ead715d4bbb173a55 I think this is because object and pointer members are mixed in these structures. So, as I mentioned on the following email [2], I should try to simplify the code instead of this patch. [2] https://marc.info/?l=linux-usb&m=155748940717665&w=2 > So are you _sure_ you want this change? I'm sorry, I would like to drop this patch. Best regards, Yoshihiro Shimoda > thanks, > > greg k-h
diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c index a501ea6..ebbe322 100644 --- a/drivers/usb/renesas_usbhs/common.c +++ b/drivers/usb/renesas_usbhs/common.c @@ -651,9 +651,8 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev) return NULL; dparam = &info->driver_param; - memcpy(dparam, &data->param, sizeof(data->param)); - memcpy(&info->platform_callback, data->platform_callback, - sizeof(*data->platform_callback)); + *dparam = data->param; + info->platform_callback = *data->platform_callback; if (!of_property_read_u32(dev->of_node, "renesas,buswait", &tmp)) dparam->buswait_bwait = tmp; @@ -714,17 +713,13 @@ static int usbhs_probe(struct platform_device *pdev) * care platform info */ - memcpy(&priv->dparam, - &info->driver_param, - sizeof(struct renesas_usbhs_driver_param)); + priv->dparam = info->driver_param; if (!info->platform_callback.get_id) { dev_err(&pdev->dev, "no platform callbacks"); return -EINVAL; } - memcpy(&priv->pfunc, - &info->platform_callback, - sizeof(struct renesas_usbhs_platform_callback)); + priv->pfunc = info->platform_callback; /* set driver callback functions for platform */ dfunc = &info->driver_callback;
To avoid the error-proneness of calls to sizeof() in the memcpy, this patch uses struct assignment instead of memcpy. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- This patch is based on Greg's linux-usb.git / usb-next branch. Note that mod_host.c also has memcpy but we cannot use struct assignment for it because the type of urb->setup_patcket is just "unsigned char *". drivers/usb/renesas_usbhs/common.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)