| Message ID | 20210516034730.621461-1-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | acf5631c239dfc53489f739c4ad47f490c5181ff |
| Headers | show |
| Series | usb: typec: mux: Fix matching with typec_altmode_desc | expand |
On Sat, May 15, 2021 at 08:47:30PM -0700, Bjorn Andersson wrote: > In typec_mux_match() "nval" is assigned the number of elements in the > "svid" fwnode property, then the variable is used to store the success > of the read and finally attempts to loop between 0 and "success" - i.e. > not at all - and the code returns indicating that no match was found. > > Fix this by using a separate variable to track the success of the read, > to allow the loop to get a change to find a match. > > Fixes: 96a6d031ca99 ("usb: typec: mux: Find the muxes by also matching against the device node") > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/mux.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c > index 9da22ae3006c..8514bec7e1b8 100644 > --- a/drivers/usb/typec/mux.c > +++ b/drivers/usb/typec/mux.c > @@ -191,6 +191,7 @@ static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id, > bool match; > int nval; > u16 *val; > + int ret; > int i; > > /* > @@ -218,10 +219,10 @@ static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id, > if (!val) > return ERR_PTR(-ENOMEM); > > - nval = fwnode_property_read_u16_array(fwnode, "svid", val, nval); > - if (nval < 0) { > + ret = fwnode_property_read_u16_array(fwnode, "svid", val, nval); > + if (ret < 0) { > kfree(val); > - return ERR_PTR(nval); > + return ERR_PTR(ret); > } > > for (i = 0; i < nval; i++) { > -- > 2.29.2 thanks,
On Sun, May 16, 2021 at 6:47 AM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > In typec_mux_match() "nval" is assigned the number of elements in the > "svid" fwnode property, then the variable is used to store the success > of the read and finally attempts to loop between 0 and "success" - i.e. > not at all - and the code returns indicating that no match was found. > > Fix this by using a separate variable to track the success of the read, > to allow the loop to get a change to find a match. > > Fixes: 96a6d031ca99 ("usb: typec: mux: Find the muxes by also matching against the device node") > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/usb/typec/mux.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c > index 9da22ae3006c..8514bec7e1b8 100644 > --- a/drivers/usb/typec/mux.c > +++ b/drivers/usb/typec/mux.c > @@ -191,6 +191,7 @@ static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id, > bool match; > int nval; > u16 *val; > + int ret; > int i; > > /* > @@ -218,10 +219,10 @@ static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id, > if (!val) > return ERR_PTR(-ENOMEM); > > - nval = fwnode_property_read_u16_array(fwnode, "svid", val, nval); > - if (nval < 0) { > + ret = fwnode_property_read_u16_array(fwnode, "svid", val, nval); > + if (ret < 0) { > kfree(val); > - return ERR_PTR(nval); > + return ERR_PTR(ret); > } This changes the behaviour of the original code, i.e. nval can be still positive but less than we got from previous call. Some fwnode backends in some cases potentially can _successfully_ read less than asked. Perhaps nval = ret; or drop the patch. > for (i = 0; i < nval; i++) {
On Mon 17 May 04:13 CDT 2021, Andy Shevchenko wrote: > On Sun, May 16, 2021 at 6:47 AM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > In typec_mux_match() "nval" is assigned the number of elements in the > > "svid" fwnode property, then the variable is used to store the success > > of the read and finally attempts to loop between 0 and "success" - i.e. > > not at all - and the code returns indicating that no match was found. > > > > Fix this by using a separate variable to track the success of the read, > > to allow the loop to get a change to find a match. > > > > Fixes: 96a6d031ca99 ("usb: typec: mux: Find the muxes by also matching against the device node") > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > drivers/usb/typec/mux.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c > > index 9da22ae3006c..8514bec7e1b8 100644 > > --- a/drivers/usb/typec/mux.c > > +++ b/drivers/usb/typec/mux.c > > @@ -191,6 +191,7 @@ static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id, > > bool match; > > int nval; > > u16 *val; > > + int ret; > > int i; > > > > /* > > @@ -218,10 +219,10 @@ static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id, > > if (!val) > > return ERR_PTR(-ENOMEM); > > > > - nval = fwnode_property_read_u16_array(fwnode, "svid", val, nval); > > - if (nval < 0) { > > + ret = fwnode_property_read_u16_array(fwnode, "svid", val, nval); > > + if (ret < 0) { > > kfree(val); > > - return ERR_PTR(nval); > > + return ERR_PTR(ret); > > } > > This changes the behaviour of the original code, i.e. nval can be > still positive but less than we got from previous call. Some fwnode > backends in some cases potentially can _successfully_ read less than > asked. > > Perhaps > > nval = ret; > > or drop the patch. > Per the kerneldoc of fwnode_property_read_u16_array: * Return: number of values if @val was %NULL, * %0 if the property was found (success), @val is not NULL, as we just checked for that, so the function will always return 0 on success. I don't see anything indicating that the number of elements can be different from what fwnode_property_count_u16() returned. Regards, Bjorn > > for (i = 0; i < nval; i++) { > > > -- > With Best Regards, > Andy Shevchenko
On Mon, May 17, 2021 at 6:14 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Mon 17 May 04:13 CDT 2021, Andy Shevchenko wrote: > > On Sun, May 16, 2021 at 6:47 AM Bjorn Andersson > > <bjorn.andersson@linaro.org> wrote: > > > > > > In typec_mux_match() "nval" is assigned the number of elements in the > > > "svid" fwnode property, then the variable is used to store the success > > > of the read and finally attempts to loop between 0 and "success" - i.e. > > > not at all - and the code returns indicating that no match was found. > > > > > > Fix this by using a separate variable to track the success of the read, > > > to allow the loop to get a change to find a match. ... > > > - nval = fwnode_property_read_u16_array(fwnode, "svid", val, nval); > > > - if (nval < 0) { > > > + ret = fwnode_property_read_u16_array(fwnode, "svid", val, nval); > > > + if (ret < 0) { > > > kfree(val); > > > - return ERR_PTR(nval); > > > + return ERR_PTR(ret); > > > } > > > > This changes the behaviour of the original code, i.e. nval can be > > still positive but less than we got from previous call. Some fwnode > > backends in some cases potentially can _successfully_ read less than > > asked. > > > > Perhaps > > > > nval = ret; > > > > or drop the patch. > > > > Per the kerneldoc of fwnode_property_read_u16_array: > > * Return: number of values if @val was %NULL, > * %0 if the property was found (success), > > @val is not NULL, as we just checked for that, so the function will > always return 0 on success. > > I don't see anything indicating that the number of elements can be > different from what fwnode_property_count_u16() returned. Okay, I have checked the backends of fwnode and indeed, OF case (from where I remember such behaviour) deliberately does if (ret >= 0) return 0; Otherwise the rest return 0 directly / explicitly. The only exception is _read_string_array(). > > > for (i = 0; i < nval; i++) {
On Mon 17 May 10:37 CDT 2021, Andy Shevchenko wrote: > On Mon, May 17, 2021 at 6:14 PM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > On Mon 17 May 04:13 CDT 2021, Andy Shevchenko wrote: > > > On Sun, May 16, 2021 at 6:47 AM Bjorn Andersson > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > In typec_mux_match() "nval" is assigned the number of elements in the > > > > "svid" fwnode property, then the variable is used to store the success > > > > of the read and finally attempts to loop between 0 and "success" - i.e. > > > > not at all - and the code returns indicating that no match was found. > > > > > > > > Fix this by using a separate variable to track the success of the read, > > > > to allow the loop to get a change to find a match. > > ... > > > > > - nval = fwnode_property_read_u16_array(fwnode, "svid", val, nval); > > > > - if (nval < 0) { > > > > + ret = fwnode_property_read_u16_array(fwnode, "svid", val, nval); > > > > + if (ret < 0) { > > > > kfree(val); > > > > - return ERR_PTR(nval); > > > > + return ERR_PTR(ret); > > > > } > > > > > > This changes the behaviour of the original code, i.e. nval can be > > > still positive but less than we got from previous call. Some fwnode > > > backends in some cases potentially can _successfully_ read less than > > > asked. > > > > > > Perhaps > > > > > > nval = ret; > > > > > > or drop the patch. > > > > > > > Per the kerneldoc of fwnode_property_read_u16_array: > > > > * Return: number of values if @val was %NULL, > > * %0 if the property was found (success), > > > > @val is not NULL, as we just checked for that, so the function will > > always return 0 on success. > > > > I don't see anything indicating that the number of elements can be > > different from what fwnode_property_count_u16() returned. > > Okay, I have checked the backends of fwnode and indeed, OF case (from > where I remember such behaviour) deliberately does > > if (ret >= 0) > return 0; > > Otherwise the rest return 0 directly / explicitly. > > The only exception is _read_string_array(). > I wasn't aware that the string array behaved difference, and the kernel-doc gives no hint either. Thanks for pointing it out. Regards, Bjorn
diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c index 9da22ae3006c..8514bec7e1b8 100644 --- a/drivers/usb/typec/mux.c +++ b/drivers/usb/typec/mux.c @@ -191,6 +191,7 @@ static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id, bool match; int nval; u16 *val; + int ret; int i; /* @@ -218,10 +219,10 @@ static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id, if (!val) return ERR_PTR(-ENOMEM); - nval = fwnode_property_read_u16_array(fwnode, "svid", val, nval); - if (nval < 0) { + ret = fwnode_property_read_u16_array(fwnode, "svid", val, nval); + if (ret < 0) { kfree(val); - return ERR_PTR(nval); + return ERR_PTR(ret); } for (i = 0; i < nval; i++) {
In typec_mux_match() "nval" is assigned the number of elements in the "svid" fwnode property, then the variable is used to store the success of the read and finally attempts to loop between 0 and "success" - i.e. not at all - and the code returns indicating that no match was found. Fix this by using a separate variable to track the success of the read, to allow the loop to get a change to find a match. Fixes: 96a6d031ca99 ("usb: typec: mux: Find the muxes by also matching against the device node") Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- drivers/usb/typec/mux.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)