Re: [PATCH] staging: ks7010: pass 'int' instead of 'bool' to 'hostif_mib_set_request_bool'
On Thu, Jul 12, 2018 at 10:11:25PM +0300, Dan Carpenter wrote:
> On Thu, Jul 12, 2018 at 08:20:16PM +0200, Sergio Paracuellos wrote:
> > On Thu, Jul 12, 2018 at 2:13 PM, Dan Carpenter
> > wrote:
> > > On Thu, Jul 12, 2018 at 06:57:42AM +0200, Sergio Paracuellos wrote:
> > >> 'hostif_mib_set_request_bool' function receives a bool as value and
> > >> send the received value with MIB_VALUE_TYPE_BOOL type. There is
> > >> one case where the value passed is not a boolean one but
> > >> 'MCAST_FILTER_PROMISC' which is '2'. Pass 'int' instead to avoid
> > >> the problem.
> > >>
> > >> Fixes: 8ce76bff0e6a ("staging: ks7010: add new helpers to achieve
> > >> mib set request and simplify code")
> > >>
> > >> Reported-by: Dan Carpenter
> > >> Signed-off-by: Sergio Paracuellos
> > >> ---
> > >> drivers/staging/ks7010/ks_hostif.c | 2 +-
> > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/staging/ks7010/ks_hostif.c
> > >> b/drivers/staging/ks7010/ks_hostif.c
> > >> index 0ecffab..6582566 100644
> > >> --- a/drivers/staging/ks7010/ks_hostif.c
> > >> +++ b/drivers/staging/ks7010/ks_hostif.c
> > >> @@ -1228,7 +1228,7 @@ static inline void
> > >> hostif_mib_set_request_int(struct ks_wlan_private *priv,
> > >>
> > >> static inline void hostif_mib_set_request_bool(struct ks_wlan_private
> > >> *priv,
> > >> enum mib_attribute attr,
> > >> -bool val)
> > >> +int val)
> > >
> > > Huh... This doesn't feel like the right thing. I thought we should
> > > change the callers to use hostif_mib_set_request_int() instead.
> >
> > Yes, I though to call that instead at first moment but I end up in revert
> > to the
> > previous behaviour...
>
> It's a tricky thing...
>
> The choices are:
>
> 1) Fix it in the CorrectWay[tm] which we both agree is
>hostif_mib_set_request_int()
>
> 2) Revert to something that looks buggy. But there is a chance it has
>been tested and works. We would hide the static checker warning
>which would make the bug harder to fix in the future.
>
> 3) Leave the code as-is and wait until someone can test it. At least
>the static checker warning is there so we will fix it eventually.
>
> I feel like we should take option 1 and if no one complains that means
> either no one is using the driver or it works. Long term that's the
> best option.
Agreed. I'll send a v2 patch calling 'hostif_mib_set_request_int'.
>
> regards,
> dan carpenter
>
Best regards,
Sergio Paracuellos
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ks7010: pass 'int' instead of 'bool' to 'hostif_mib_set_request_bool'
On Thu, Jul 12, 2018 at 08:20:16PM +0200, Sergio Paracuellos wrote:
> On Thu, Jul 12, 2018 at 2:13 PM, Dan Carpenter
> wrote:
> > On Thu, Jul 12, 2018 at 06:57:42AM +0200, Sergio Paracuellos wrote:
> >> 'hostif_mib_set_request_bool' function receives a bool as value and
> >> send the received value with MIB_VALUE_TYPE_BOOL type. There is
> >> one case where the value passed is not a boolean one but
> >> 'MCAST_FILTER_PROMISC' which is '2'. Pass 'int' instead to avoid
> >> the problem.
> >>
> >> Fixes: 8ce76bff0e6a ("staging: ks7010: add new helpers to achieve
> >> mib set request and simplify code")
> >>
> >> Reported-by: Dan Carpenter
> >> Signed-off-by: Sergio Paracuellos
> >> ---
> >> drivers/staging/ks7010/ks_hostif.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/staging/ks7010/ks_hostif.c
> >> b/drivers/staging/ks7010/ks_hostif.c
> >> index 0ecffab..6582566 100644
> >> --- a/drivers/staging/ks7010/ks_hostif.c
> >> +++ b/drivers/staging/ks7010/ks_hostif.c
> >> @@ -1228,7 +1228,7 @@ static inline void hostif_mib_set_request_int(struct
> >> ks_wlan_private *priv,
> >>
> >> static inline void hostif_mib_set_request_bool(struct ks_wlan_private
> >> *priv,
> >> enum mib_attribute attr,
> >> -bool val)
> >> +int val)
> >
> > Huh... This doesn't feel like the right thing. I thought we should
> > change the callers to use hostif_mib_set_request_int() instead.
>
> Yes, I though to call that instead at first moment but I end up in revert to
> the
> previous behaviour...
It's a tricky thing...
The choices are:
1) Fix it in the CorrectWay[tm] which we both agree is
hostif_mib_set_request_int()
2) Revert to something that looks buggy. But there is a chance it has
been tested and works. We would hide the static checker warning
which would make the bug harder to fix in the future.
3) Leave the code as-is and wait until someone can test it. At least
the static checker warning is there so we will fix it eventually.
I feel like we should take option 1 and if no one complains that means
either no one is using the driver or it works. Long term that's the
best option.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ks7010: pass 'int' instead of 'bool' to 'hostif_mib_set_request_bool'
On Thu, Jul 12, 2018 at 2:13 PM, Dan Carpenter wrote:
> On Thu, Jul 12, 2018 at 06:57:42AM +0200, Sergio Paracuellos wrote:
>> 'hostif_mib_set_request_bool' function receives a bool as value and
>> send the received value with MIB_VALUE_TYPE_BOOL type. There is
>> one case where the value passed is not a boolean one but
>> 'MCAST_FILTER_PROMISC' which is '2'. Pass 'int' instead to avoid
>> the problem.
>>
>> Fixes: 8ce76bff0e6a ("staging: ks7010: add new helpers to achieve
>> mib set request and simplify code")
>>
>> Reported-by: Dan Carpenter
>> Signed-off-by: Sergio Paracuellos
>> ---
>> drivers/staging/ks7010/ks_hostif.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/ks7010/ks_hostif.c
>> b/drivers/staging/ks7010/ks_hostif.c
>> index 0ecffab..6582566 100644
>> --- a/drivers/staging/ks7010/ks_hostif.c
>> +++ b/drivers/staging/ks7010/ks_hostif.c
>> @@ -1228,7 +1228,7 @@ static inline void hostif_mib_set_request_int(struct
>> ks_wlan_private *priv,
>>
>> static inline void hostif_mib_set_request_bool(struct ks_wlan_private *priv,
>> enum mib_attribute attr,
>> -bool val)
>> +int val)
>
> Huh... This doesn't feel like the right thing. I thought we should
> change the callers to use hostif_mib_set_request_int() instead.
Yes, I though to call that instead at first moment but I end up in revert to the
previous behaviour...
>
> This reverts to the original behavior, yes, but I think the original was
> wrong. This is one of those changes which requires someone to test it
> probably. Or at least understand the code more than you and I do...
>
I think is wrong also but I cannot be sure so... just being
conservative this time :)
> regards,
> dan carpenter
>
Best regards,
Sergio Paracuellos
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ks7010: pass 'int' instead of 'bool' to 'hostif_mib_set_request_bool'
On Thu, Jul 12, 2018 at 06:57:42AM +0200, Sergio Paracuellos wrote:
> 'hostif_mib_set_request_bool' function receives a bool as value and
> send the received value with MIB_VALUE_TYPE_BOOL type. There is
> one case where the value passed is not a boolean one but
> 'MCAST_FILTER_PROMISC' which is '2'. Pass 'int' instead to avoid
> the problem.
>
> Fixes: 8ce76bff0e6a ("staging: ks7010: add new helpers to achieve
> mib set request and simplify code")
>
> Reported-by: Dan Carpenter
> Signed-off-by: Sergio Paracuellos
> ---
> drivers/staging/ks7010/ks_hostif.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/ks7010/ks_hostif.c
> b/drivers/staging/ks7010/ks_hostif.c
> index 0ecffab..6582566 100644
> --- a/drivers/staging/ks7010/ks_hostif.c
> +++ b/drivers/staging/ks7010/ks_hostif.c
> @@ -1228,7 +1228,7 @@ static inline void hostif_mib_set_request_int(struct
> ks_wlan_private *priv,
>
> static inline void hostif_mib_set_request_bool(struct ks_wlan_private *priv,
> enum mib_attribute attr,
> -bool val)
> +int val)
Huh... This doesn't feel like the right thing. I thought we should
change the callers to use hostif_mib_set_request_int() instead.
This reverts to the original behavior, yes, but I think the original was
wrong. This is one of those changes which requires someone to test it
probably. Or at least understand the code more than you and I do...
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
