| Message ID | 20210317142513.1340-1-jzp0409@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: acpi: Fix shifting 31 bits | expand |
On Wed, Mar 17, 2021 at 10:25:13PM +0800, jzp0409 wrote: > From: "edison.jiang" <jiangzhipeng@yulong.com> > > Fix undefined behaviour in the usb apci driver by using 'BIT' marcro. What is undefined about it? > > Signed-off-by: edison.jiang <jiangzhipeng@yulong.com> This name is not a valid signed-off-by name (I doubt you sign documents with a ".", right?) and it does not match the "From:" name either, so something needs to be fixed before this patch could be accepted. > --- > drivers/usb/core/usb-acpi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c > index 50b2fc7..3e467a8 100644 > --- a/drivers/usb/core/usb-acpi.c > +++ b/drivers/usb/core/usb-acpi.c > @@ -122,7 +122,7 @@ static enum usb_port_connect_type usb_acpi_get_connect_type(acpi_handle handle, > * Private to usb-acpi, all the core needs to know is that > * port_dev->location is non-zero when it has been set by the firmware. > */ > -#define USB_ACPI_LOCATION_VALID (1 << 31) > +#define USB_ACPI_LOCATION_VALID BIT(31) I do not understand what this is trying to fix, please be more specific. thanks, greg k-h
On Wed, 17 Mar 2021 15:51:02 +0100 Greg KH <gregkh@linuxfoundation.org> wrote: > On Wed, Mar 17, 2021 at 10:25:13PM +0800, jzp0409 wrote: > > From: "edison.jiang" <jiangzhipeng@yulong.com> > > > > Fix undefined behaviour in the usb apci driver by using 'BIT' > > marcro. > > What is undefined about it? > > > > > Signed-off-by: edison.jiang <jiangzhipeng@yulong.com> > > This name is not a valid signed-off-by name (I doubt you sign > documents with a ".", right?) and it does not match the "From:" name > either, so something needs to be fixed before this patch could be > accepted. > > > --- > > drivers/usb/core/usb-acpi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/usb/core/usb-acpi.c > > b/drivers/usb/core/usb-acpi.c index 50b2fc7..3e467a8 100644 > > --- a/drivers/usb/core/usb-acpi.c > > +++ b/drivers/usb/core/usb-acpi.c > > @@ -122,7 +122,7 @@ static enum usb_port_connect_type > > usb_acpi_get_connect_type(acpi_handle handle, > > * Private to usb-acpi, all the core needs to know is that > > * port_dev->location is non-zero when it has been set by the > > firmware. */ > > -#define USB_ACPI_LOCATION_VALID (1 << 31) > > +#define USB_ACPI_LOCATION_VALID BIT(31) > > I do not understand what this is trying to fix, please be more > specific. > > thanks, > > greg k-h cppcheck error: linux/drivers/usb/core/usb-acpi.c:191]: (error) Shifting signed 32-bit value by 31 bits is undefined behaviour
On Thu, Mar 18, 2021 at 10:14:30AM +0800, jiangzhipeng wrote: > On Wed, 17 Mar 2021 15:51:02 +0100 > Greg KH <gregkh@linuxfoundation.org> wrote: > > > On Wed, Mar 17, 2021 at 10:25:13PM +0800, jzp0409 wrote: > > > From: "edison.jiang" <jiangzhipeng@yulong.com> > > > > > > Fix undefined behaviour in the usb apci driver by using 'BIT' > > > marcro. > > > > What is undefined about it? > > > > > > > > Signed-off-by: edison.jiang <jiangzhipeng@yulong.com> > > > > This name is not a valid signed-off-by name (I doubt you sign > > documents with a ".", right?) and it does not match the "From:" name > > either, so something needs to be fixed before this patch could be > > accepted. > > > > > --- > > > drivers/usb/core/usb-acpi.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/core/usb-acpi.c > > > b/drivers/usb/core/usb-acpi.c index 50b2fc7..3e467a8 100644 > > > --- a/drivers/usb/core/usb-acpi.c > > > +++ b/drivers/usb/core/usb-acpi.c > > > @@ -122,7 +122,7 @@ static enum usb_port_connect_type > > > usb_acpi_get_connect_type(acpi_handle handle, > > > * Private to usb-acpi, all the core needs to know is that > > > * port_dev->location is non-zero when it has been set by the > > > firmware. */ > > > -#define USB_ACPI_LOCATION_VALID (1 << 31) > > > +#define USB_ACPI_LOCATION_VALID BIT(31) > > > > I do not understand what this is trying to fix, please be more > > specific. > > > > thanks, > > > > greg k-h > > cppcheck error: > linux/drivers/usb/core/usb-acpi.c:191]: (error) Shifting signed 32-bit > value by 31 bits is undefined behaviour > That does not explain anything, sorry. greg k-h
On Thu, Mar 18, 2021 at 06:27:37AM +0100, Greg KH wrote: > On Thu, Mar 18, 2021 at 10:14:30AM +0800, jiangzhipeng wrote: > > On Wed, 17 Mar 2021 15:51:02 +0100 > > Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > On Wed, Mar 17, 2021 at 10:25:13PM +0800, jzp0409 wrote: > > > > From: "edison.jiang" <jiangzhipeng@yulong.com> > > > > > > > > Fix undefined behaviour in the usb apci driver by using 'BIT' > > > > marcro. > > > > > > What is undefined about it? > > > > > > > > > > > Signed-off-by: edison.jiang <jiangzhipeng@yulong.com> > > > > > > This name is not a valid signed-off-by name (I doubt you sign > > > documents with a ".", right?) and it does not match the "From:" name > > > either, so something needs to be fixed before this patch could be > > > accepted. > > > > > > > --- > > > > drivers/usb/core/usb-acpi.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/usb/core/usb-acpi.c > > > > b/drivers/usb/core/usb-acpi.c index 50b2fc7..3e467a8 100644 > > > > --- a/drivers/usb/core/usb-acpi.c > > > > +++ b/drivers/usb/core/usb-acpi.c > > > > @@ -122,7 +122,7 @@ static enum usb_port_connect_type > > > > usb_acpi_get_connect_type(acpi_handle handle, > > > > * Private to usb-acpi, all the core needs to know is that > > > > * port_dev->location is non-zero when it has been set by the > > > > firmware. */ > > > > -#define USB_ACPI_LOCATION_VALID (1 << 31) > > > > +#define USB_ACPI_LOCATION_VALID BIT(31) > > > > > > I do not understand what this is trying to fix, please be more > > > specific. > > > > > > thanks, > > > > > > greg k-h > > > > cppcheck error: > > linux/drivers/usb/core/usb-acpi.c:191]: (error) Shifting signed 32-bit > > value by 31 bits is undefined behaviour > > > > That does not explain anything, sorry. In other words, cppcheck is usually horrid and you need to do some real work to determine what it is trying to say, and to see if it just is wrong or not. Please do that research and if there is a real issue, submit a patch to resolve it and put that information in the changelog text to explain what is happening. As it is, this change doesn't seem to actually do anything except quiet a random tool. And that tool is wrong here. thanks, greg k-h
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c index 50b2fc7..3e467a8 100644 --- a/drivers/usb/core/usb-acpi.c +++ b/drivers/usb/core/usb-acpi.c @@ -122,7 +122,7 @@ static enum usb_port_connect_type usb_acpi_get_connect_type(acpi_handle handle, * Private to usb-acpi, all the core needs to know is that * port_dev->location is non-zero when it has been set by the firmware. */ -#define USB_ACPI_LOCATION_VALID (1 << 31) +#define USB_ACPI_LOCATION_VALID BIT(31) static struct acpi_device *usb_acpi_find_port(struct acpi_device *parent, int raw)