Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change
On Wed, Jun 20, 2018 at 12:46 AM, Daniel Drake wrote:
> Hi,
>
> On Thu, Jun 14, 2018 at 1:58 AM, Chris Chiu wrote:
>>
>> On Wed, Jun 13, 2018 at 8:49 PM, Andy Shevchenko
>> wrote:
>> > On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu wrote:
>> >> Make asus-wmi notify on hotkey kbd brightness changes, listen for
>> >> brightness events and update the brightness directly in the driver.
>> >
>> >> For this purpose, bound check on brightness in kbd_led_set must be
>> >> based on the same data type to prevent illegal value been set.
>> >
>> >> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>> >>
>> >> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>> >>
>> >> - if (value > asus->kbd_led.max_brightness)
>> >> + if ((int)value > (int)asus->kbd_led.max_brightness)
>> >> value = asus->kbd_led.max_brightness;
>> >> - else if (value < 0)
>> >> + else if ((int)value < 0)
>> >> value = 0;
>> >
>> > I didn't quite understand this part of the problem.
>> > Does it exist in the current code? Can it be split to a separate change?
>> >
>> > Can we avoid those ugly castings?
>> >
>>
>> I'd like to remove the ugly castings but there's a concern I may need some
>> advices. I don't know whether if the bound check logic ever verified before.
>> Maybe the value passed via sysfs is already correctly bounded?
>
> The casts come from the underlying need to limit the minumum and
> maximum brightness within available bounds, plus difficulties doing
> that when you are dealing with an enum data type.
>
> kbd_led_set is a function pointer (from led_classdev.brightness_set)
> which has this definition:
>
> void (*brightness_set)(struct led_classdev *led_cdev, enum
> led_brightness brightness);
>
> It seems that the compiler has the choice of whether to use a signed
> or unsigned type for enums. According to your tests, and a quick test
> app below, it seems like gcc is using unsigned.
>
> #include
> enum led_brightness { LED_OFF = 0 };
> int main(void) {
> enum led_brightness tmp = -1;
> if (tmp < 0)
> printf("less than zero\n");
> else
> printf("gt zero\n");
> }
>
> This test app prints "gt zero"
>
> led-class.c:brightness_store() uses kstrtoul() so there is no chance
> of passing a negative value through this codepath, as you suspected.
> But we do need to do something with negative bounds for the code that
> you are adding here.
>
> I suggest doing it like this:
>
> static void __kbd_led_set(struct led_classdev *led_cdev, int value)
> {
> struct asus_wmi *asus;
>
> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>
> if (value > asus->kbd_led.max_brightness)
> value = asus->kbd_led.max_brightness;
The `value > asus->kbd_led.max_brightness` will still return false here.
So I would modify as follows in v3.
int max_level = asus->kbd_led.max_brightness;
if (value > max_level)
value = max_level;
I've verified there's no regression on led_classdev call path via sysfs.
> else if (value < 0)
> value = 0;
>
> asus->kbd_led_wk = value;
> queue_work(asus->led_workqueue, >kbd_led_work);
> }
>
> static void kbd_led_set(struct led_classdev *led_cdev,
> enum led_brightness value)
> {
> return __kbd_led_set(led_cdev, value);
> }
>
> Now kbd_led_set can continue being a correctly typed function pointer
> for led_classdev.brightness_set. And from the code you are adding here
> you can call __kbd_led_set directly with signed integer values, and
> rely on correct bounds correction without ugly casts.
>
> Andy, what do you think?
>
> Thanks
> Daniel
Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change
On Wed, Jun 20, 2018 at 12:46 AM, Daniel Drake wrote:
> Hi,
>
> On Thu, Jun 14, 2018 at 1:58 AM, Chris Chiu wrote:
>>
>> On Wed, Jun 13, 2018 at 8:49 PM, Andy Shevchenko
>> wrote:
>> > On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu wrote:
>> >> Make asus-wmi notify on hotkey kbd brightness changes, listen for
>> >> brightness events and update the brightness directly in the driver.
>> >
>> >> For this purpose, bound check on brightness in kbd_led_set must be
>> >> based on the same data type to prevent illegal value been set.
>> >
>> >> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>> >>
>> >> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>> >>
>> >> - if (value > asus->kbd_led.max_brightness)
>> >> + if ((int)value > (int)asus->kbd_led.max_brightness)
>> >> value = asus->kbd_led.max_brightness;
>> >> - else if (value < 0)
>> >> + else if ((int)value < 0)
>> >> value = 0;
>> >
>> > I didn't quite understand this part of the problem.
>> > Does it exist in the current code? Can it be split to a separate change?
>> >
>> > Can we avoid those ugly castings?
>> >
>>
>> I'd like to remove the ugly castings but there's a concern I may need some
>> advices. I don't know whether if the bound check logic ever verified before.
>> Maybe the value passed via sysfs is already correctly bounded?
>
> The casts come from the underlying need to limit the minumum and
> maximum brightness within available bounds, plus difficulties doing
> that when you are dealing with an enum data type.
>
> kbd_led_set is a function pointer (from led_classdev.brightness_set)
> which has this definition:
>
> void (*brightness_set)(struct led_classdev *led_cdev, enum
> led_brightness brightness);
>
> It seems that the compiler has the choice of whether to use a signed
> or unsigned type for enums. According to your tests, and a quick test
> app below, it seems like gcc is using unsigned.
>
> #include
> enum led_brightness { LED_OFF = 0 };
> int main(void) {
> enum led_brightness tmp = -1;
> if (tmp < 0)
> printf("less than zero\n");
> else
> printf("gt zero\n");
> }
>
> This test app prints "gt zero"
>
> led-class.c:brightness_store() uses kstrtoul() so there is no chance
> of passing a negative value through this codepath, as you suspected.
> But we do need to do something with negative bounds for the code that
> you are adding here.
>
> I suggest doing it like this:
>
> static void __kbd_led_set(struct led_classdev *led_cdev, int value)
> {
> struct asus_wmi *asus;
>
> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>
> if (value > asus->kbd_led.max_brightness)
> value = asus->kbd_led.max_brightness;
The `value > asus->kbd_led.max_brightness` will still return false here.
So I would modify as follows in v3.
int max_level = asus->kbd_led.max_brightness;
if (value > max_level)
value = max_level;
I've verified there's no regression on led_classdev call path via sysfs.
> else if (value < 0)
> value = 0;
>
> asus->kbd_led_wk = value;
> queue_work(asus->led_workqueue, >kbd_led_work);
> }
>
> static void kbd_led_set(struct led_classdev *led_cdev,
> enum led_brightness value)
> {
> return __kbd_led_set(led_cdev, value);
> }
>
> Now kbd_led_set can continue being a correctly typed function pointer
> for led_classdev.brightness_set. And from the code you are adding here
> you can call __kbd_led_set directly with signed integer values, and
> rely on correct bounds correction without ugly casts.
>
> Andy, what do you think?
>
> Thanks
> Daniel
Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change
On Tue, Jun 19, 2018 at 7:46 PM, Daniel Drake wrote: >> > Can we avoid those ugly castings? > Now kbd_led_set can continue being a correctly typed function pointer > for led_classdev.brightness_set. And from the code you are adding here > you can call __kbd_led_set directly with signed integer values, and > rely on correct bounds correction without ugly casts. > > Andy, what do you think? Thanks, Daniel for investigation. I'm fine with solution(s) that has no ugly casting and no regression on the hardware we support. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change
On Tue, Jun 19, 2018 at 7:46 PM, Daniel Drake wrote: >> > Can we avoid those ugly castings? > Now kbd_led_set can continue being a correctly typed function pointer > for led_classdev.brightness_set. And from the code you are adding here > you can call __kbd_led_set directly with signed integer values, and > rely on correct bounds correction without ugly casts. > > Andy, what do you think? Thanks, Daniel for investigation. I'm fine with solution(s) that has no ugly casting and no regression on the hardware we support. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change
Hi,
On Thu, Jun 14, 2018 at 1:58 AM, Chris Chiu wrote:
>
> On Wed, Jun 13, 2018 at 8:49 PM, Andy Shevchenko
> wrote:
> > On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu wrote:
> >> Make asus-wmi notify on hotkey kbd brightness changes, listen for
> >> brightness events and update the brightness directly in the driver.
> >
> >> For this purpose, bound check on brightness in kbd_led_set must be
> >> based on the same data type to prevent illegal value been set.
> >
> >> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
> >>
> >> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
> >>
> >> - if (value > asus->kbd_led.max_brightness)
> >> + if ((int)value > (int)asus->kbd_led.max_brightness)
> >> value = asus->kbd_led.max_brightness;
> >> - else if (value < 0)
> >> + else if ((int)value < 0)
> >> value = 0;
> >
> > I didn't quite understand this part of the problem.
> > Does it exist in the current code? Can it be split to a separate change?
> >
> > Can we avoid those ugly castings?
> >
>
> I'd like to remove the ugly castings but there's a concern I may need some
> advices. I don't know whether if the bound check logic ever verified before.
> Maybe the value passed via sysfs is already correctly bounded?
The casts come from the underlying need to limit the minumum and
maximum brightness within available bounds, plus difficulties doing
that when you are dealing with an enum data type.
kbd_led_set is a function pointer (from led_classdev.brightness_set)
which has this definition:
void (*brightness_set)(struct led_classdev *led_cdev, enum
led_brightness brightness);
It seems that the compiler has the choice of whether to use a signed
or unsigned type for enums. According to your tests, and a quick test
app below, it seems like gcc is using unsigned.
#include
enum led_brightness { LED_OFF = 0 };
int main(void) {
enum led_brightness tmp = -1;
if (tmp < 0)
printf("less than zero\n");
else
printf("gt zero\n");
}
This test app prints "gt zero"
led-class.c:brightness_store() uses kstrtoul() so there is no chance
of passing a negative value through this codepath, as you suspected.
But we do need to do something with negative bounds for the code that
you are adding here.
I suggest doing it like this:
static void __kbd_led_set(struct led_classdev *led_cdev, int value)
{
struct asus_wmi *asus;
asus = container_of(led_cdev, struct asus_wmi, kbd_led);
if (value > asus->kbd_led.max_brightness)
value = asus->kbd_led.max_brightness;
else if (value < 0)
value = 0;
asus->kbd_led_wk = value;
queue_work(asus->led_workqueue, >kbd_led_work);
}
static void kbd_led_set(struct led_classdev *led_cdev,
enum led_brightness value)
{
return __kbd_led_set(led_cdev, value);
}
Now kbd_led_set can continue being a correctly typed function pointer
for led_classdev.brightness_set. And from the code you are adding here
you can call __kbd_led_set directly with signed integer values, and
rely on correct bounds correction without ugly casts.
Andy, what do you think?
Thanks
Daniel
Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change
Hi,
On Thu, Jun 14, 2018 at 1:58 AM, Chris Chiu wrote:
>
> On Wed, Jun 13, 2018 at 8:49 PM, Andy Shevchenko
> wrote:
> > On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu wrote:
> >> Make asus-wmi notify on hotkey kbd brightness changes, listen for
> >> brightness events and update the brightness directly in the driver.
> >
> >> For this purpose, bound check on brightness in kbd_led_set must be
> >> based on the same data type to prevent illegal value been set.
> >
> >> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
> >>
> >> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
> >>
> >> - if (value > asus->kbd_led.max_brightness)
> >> + if ((int)value > (int)asus->kbd_led.max_brightness)
> >> value = asus->kbd_led.max_brightness;
> >> - else if (value < 0)
> >> + else if ((int)value < 0)
> >> value = 0;
> >
> > I didn't quite understand this part of the problem.
> > Does it exist in the current code? Can it be split to a separate change?
> >
> > Can we avoid those ugly castings?
> >
>
> I'd like to remove the ugly castings but there's a concern I may need some
> advices. I don't know whether if the bound check logic ever verified before.
> Maybe the value passed via sysfs is already correctly bounded?
The casts come from the underlying need to limit the minumum and
maximum brightness within available bounds, plus difficulties doing
that when you are dealing with an enum data type.
kbd_led_set is a function pointer (from led_classdev.brightness_set)
which has this definition:
void (*brightness_set)(struct led_classdev *led_cdev, enum
led_brightness brightness);
It seems that the compiler has the choice of whether to use a signed
or unsigned type for enums. According to your tests, and a quick test
app below, it seems like gcc is using unsigned.
#include
enum led_brightness { LED_OFF = 0 };
int main(void) {
enum led_brightness tmp = -1;
if (tmp < 0)
printf("less than zero\n");
else
printf("gt zero\n");
}
This test app prints "gt zero"
led-class.c:brightness_store() uses kstrtoul() so there is no chance
of passing a negative value through this codepath, as you suspected.
But we do need to do something with negative bounds for the code that
you are adding here.
I suggest doing it like this:
static void __kbd_led_set(struct led_classdev *led_cdev, int value)
{
struct asus_wmi *asus;
asus = container_of(led_cdev, struct asus_wmi, kbd_led);
if (value > asus->kbd_led.max_brightness)
value = asus->kbd_led.max_brightness;
else if (value < 0)
value = 0;
asus->kbd_led_wk = value;
queue_work(asus->led_workqueue, >kbd_led_work);
}
static void kbd_led_set(struct led_classdev *led_cdev,
enum led_brightness value)
{
return __kbd_led_set(led_cdev, value);
}
Now kbd_led_set can continue being a correctly typed function pointer
for led_classdev.brightness_set. And from the code you are adding here
you can call __kbd_led_set directly with signed integer values, and
rely on correct bounds correction without ugly casts.
Andy, what do you think?
Thanks
Daniel
Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change
On Thu, Jun 14, 2018 at 2:58 PM, Chris Chiu wrote: > On Wed, Jun 13, 2018 at 8:49 PM, Andy Shevchenko > wrote: >> On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu wrote: >>> Make asus-wmi notify on hotkey kbd brightness changes, listen for >>> brightness events and update the brightness directly in the driver. >> >>> For this purpose, bound check on brightness in kbd_led_set must be >>> based on the same data type to prevent illegal value been set. >> >>> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev, >>> >>> asus = container_of(led_cdev, struct asus_wmi, kbd_led); >>> >>> - if (value > asus->kbd_led.max_brightness) >>> + if ((int)value > (int)asus->kbd_led.max_brightness) >>> value = asus->kbd_led.max_brightness; >>> - else if (value < 0) >>> + else if ((int)value < 0) >>> value = 0; >> >> I didn't quite understand this part of the problem. >> Does it exist in the current code? Can it be split to a separate change? >> >> Can we avoid those ugly castings? >> > > I'd like to remove the ugly castings but there's a concern I may need some > advices. I don't know whether if the bound check logic ever verified before. > Maybe the value passed via sysfs is already correctly bounded? > > When the kbd_led_wk comes to -1, `if (value > asus->kbd_led.max_brightness)` > returns true and `if (value < 0)` return false which confused me. It > should relate > to the 2nd argument type is enum led_brightness which I consider it as > integer. > The unexpected return value cause the KBD_BRTDWN cyclic, 3->2->0->-1 > (3 again in kbd_led_set)->2->1. After applying the casting on both operands > `if ((int)value > (int)asus->kbd_led.max_brightness)`, the other > unexpected false > returned by `if (value < 0)` makes each KBD_BRTDOWN to be 3 -> 2 -> 1 -> 0 -> > -1 -> -2 -> -3 ->. That's what the ugly casting for. I used to > tend to do boundary > limit before calling kbd_led_set as follows > > kbd_led_set(>kbd_led, MIN(asus->kbd_led_wk + 1, > asus->kbd_led.max_brightness); > and > kbd_led_set(>kbd_led, MAX(asus->kbd_led_wk - 1, 0)); > > If so, the boundary limit logic would be totally redundant but I think > it may be still > useful to check the value passed from sysfs? Any suggestion which one would > be better? > > Chris > >> -- >> With Best Regards, >> Andy Shevchenko Gentle ping. Cheers. Chris
Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change
On Thu, Jun 14, 2018 at 2:58 PM, Chris Chiu wrote: > On Wed, Jun 13, 2018 at 8:49 PM, Andy Shevchenko > wrote: >> On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu wrote: >>> Make asus-wmi notify on hotkey kbd brightness changes, listen for >>> brightness events and update the brightness directly in the driver. >> >>> For this purpose, bound check on brightness in kbd_led_set must be >>> based on the same data type to prevent illegal value been set. >> >>> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev, >>> >>> asus = container_of(led_cdev, struct asus_wmi, kbd_led); >>> >>> - if (value > asus->kbd_led.max_brightness) >>> + if ((int)value > (int)asus->kbd_led.max_brightness) >>> value = asus->kbd_led.max_brightness; >>> - else if (value < 0) >>> + else if ((int)value < 0) >>> value = 0; >> >> I didn't quite understand this part of the problem. >> Does it exist in the current code? Can it be split to a separate change? >> >> Can we avoid those ugly castings? >> > > I'd like to remove the ugly castings but there's a concern I may need some > advices. I don't know whether if the bound check logic ever verified before. > Maybe the value passed via sysfs is already correctly bounded? > > When the kbd_led_wk comes to -1, `if (value > asus->kbd_led.max_brightness)` > returns true and `if (value < 0)` return false which confused me. It > should relate > to the 2nd argument type is enum led_brightness which I consider it as > integer. > The unexpected return value cause the KBD_BRTDWN cyclic, 3->2->0->-1 > (3 again in kbd_led_set)->2->1. After applying the casting on both operands > `if ((int)value > (int)asus->kbd_led.max_brightness)`, the other > unexpected false > returned by `if (value < 0)` makes each KBD_BRTDOWN to be 3 -> 2 -> 1 -> 0 -> > -1 -> -2 -> -3 ->. That's what the ugly casting for. I used to > tend to do boundary > limit before calling kbd_led_set as follows > > kbd_led_set(>kbd_led, MIN(asus->kbd_led_wk + 1, > asus->kbd_led.max_brightness); > and > kbd_led_set(>kbd_led, MAX(asus->kbd_led_wk - 1, 0)); > > If so, the boundary limit logic would be totally redundant but I think > it may be still > useful to check the value passed from sysfs? Any suggestion which one would > be better? > > Chris > >> -- >> With Best Regards, >> Andy Shevchenko Gentle ping. Cheers. Chris
Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change
On Wed, Jun 13, 2018 at 8:49 PM, Andy Shevchenko wrote: > On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu wrote: >> Make asus-wmi notify on hotkey kbd brightness changes, listen for >> brightness events and update the brightness directly in the driver. > >> For this purpose, bound check on brightness in kbd_led_set must be >> based on the same data type to prevent illegal value been set. > >> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev, >> >> asus = container_of(led_cdev, struct asus_wmi, kbd_led); >> >> - if (value > asus->kbd_led.max_brightness) >> + if ((int)value > (int)asus->kbd_led.max_brightness) >> value = asus->kbd_led.max_brightness; >> - else if (value < 0) >> + else if ((int)value < 0) >> value = 0; > > I didn't quite understand this part of the problem. > Does it exist in the current code? Can it be split to a separate change? > > Can we avoid those ugly castings? > I'd like to remove the ugly castings but there's a concern I may need some advices. I don't know whether if the bound check logic ever verified before. Maybe the value passed via sysfs is already correctly bounded? When the kbd_led_wk comes to -1, `if (value > asus->kbd_led.max_brightness)` returns true and `if (value < 0)` return false which confused me. It should relate to the 2nd argument type is enum led_brightness which I consider it as integer. The unexpected return value cause the KBD_BRTDWN cyclic, 3->2->0->-1 (3 again in kbd_led_set)->2->1. After applying the casting on both operands `if ((int)value > (int)asus->kbd_led.max_brightness)`, the other unexpected false returned by `if (value < 0)` makes each KBD_BRTDOWN to be 3 -> 2 -> 1 -> 0 -> -1 -> -2 -> -3 ->. That's what the ugly casting for. I used to tend to do boundary limit before calling kbd_led_set as follows kbd_led_set(>kbd_led, MIN(asus->kbd_led_wk + 1, asus->kbd_led.max_brightness); and kbd_led_set(>kbd_led, MAX(asus->kbd_led_wk - 1, 0)); If so, the boundary limit logic would be totally redundant but I think it may be still useful to check the value passed from sysfs? Any suggestion which one would be better? Chris > -- > With Best Regards, > Andy Shevchenko
Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change
On Wed, Jun 13, 2018 at 8:49 PM, Andy Shevchenko wrote: > On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu wrote: >> Make asus-wmi notify on hotkey kbd brightness changes, listen for >> brightness events and update the brightness directly in the driver. > >> For this purpose, bound check on brightness in kbd_led_set must be >> based on the same data type to prevent illegal value been set. > >> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev, >> >> asus = container_of(led_cdev, struct asus_wmi, kbd_led); >> >> - if (value > asus->kbd_led.max_brightness) >> + if ((int)value > (int)asus->kbd_led.max_brightness) >> value = asus->kbd_led.max_brightness; >> - else if (value < 0) >> + else if ((int)value < 0) >> value = 0; > > I didn't quite understand this part of the problem. > Does it exist in the current code? Can it be split to a separate change? > > Can we avoid those ugly castings? > I'd like to remove the ugly castings but there's a concern I may need some advices. I don't know whether if the bound check logic ever verified before. Maybe the value passed via sysfs is already correctly bounded? When the kbd_led_wk comes to -1, `if (value > asus->kbd_led.max_brightness)` returns true and `if (value < 0)` return false which confused me. It should relate to the 2nd argument type is enum led_brightness which I consider it as integer. The unexpected return value cause the KBD_BRTDWN cyclic, 3->2->0->-1 (3 again in kbd_led_set)->2->1. After applying the casting on both operands `if ((int)value > (int)asus->kbd_led.max_brightness)`, the other unexpected false returned by `if (value < 0)` makes each KBD_BRTDOWN to be 3 -> 2 -> 1 -> 0 -> -1 -> -2 -> -3 ->. That's what the ugly casting for. I used to tend to do boundary limit before calling kbd_led_set as follows kbd_led_set(>kbd_led, MIN(asus->kbd_led_wk + 1, asus->kbd_led.max_brightness); and kbd_led_set(>kbd_led, MAX(asus->kbd_led_wk - 1, 0)); If so, the boundary limit logic would be totally redundant but I think it may be still useful to check the value passed from sysfs? Any suggestion which one would be better? Chris > -- > With Best Regards, > Andy Shevchenko
Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change
On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu wrote: > Make asus-wmi notify on hotkey kbd brightness changes, listen for > brightness events and update the brightness directly in the driver. > For this purpose, bound check on brightness in kbd_led_set must be > based on the same data type to prevent illegal value been set. > @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev, > > asus = container_of(led_cdev, struct asus_wmi, kbd_led); > > - if (value > asus->kbd_led.max_brightness) > + if ((int)value > (int)asus->kbd_led.max_brightness) > value = asus->kbd_led.max_brightness; > - else if (value < 0) > + else if ((int)value < 0) > value = 0; I didn't quite understand this part of the problem. Does it exist in the current code? Can it be split to a separate change? Can we avoid those ugly castings? -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change
On Mon, Jun 11, 2018 at 10:18 AM, Chris Chiu wrote: > Make asus-wmi notify on hotkey kbd brightness changes, listen for > brightness events and update the brightness directly in the driver. > For this purpose, bound check on brightness in kbd_led_set must be > based on the same data type to prevent illegal value been set. > @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev, > > asus = container_of(led_cdev, struct asus_wmi, kbd_led); > > - if (value > asus->kbd_led.max_brightness) > + if ((int)value > (int)asus->kbd_led.max_brightness) > value = asus->kbd_led.max_brightness; > - else if (value < 0) > + else if ((int)value < 0) > value = 0; I didn't quite understand this part of the problem. Does it exist in the current code? Can it be split to a separate change? Can we avoid those ugly castings? -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change
On Mon, Jun 11, 2018 at 3:18 PM, Chris Chiu wrote:
> Make asus-wmi notify on hotkey kbd brightness changes, listen for
> brightness events and update the brightness directly in the driver.
> For this purpose, bound check on brightness in kbd_led_set must be
> based on the same data type to prevent illegal value been set.
>
> Update the brightness by led_classdev_notify_brightness_hw_changed.
> This will allow userspace to monitor (poll) for brightness changes
> on the LED without reporting via input keymapping.
>
> Signed-off-by: Chris Chiu
> ---
> drivers/platform/x86/asus-wmi.c | 15 +--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 1f6e68f0b646..a18484f4bae3 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -460,6 +460,7 @@ static void kbd_led_update(struct work_struct *work)
> ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
>
> asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
> + led_classdev_notify_brightness_hw_changed(>kbd_led,
> asus->kbd_led_wk);
> }
>
> static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>
> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>
> - if (value > asus->kbd_led.max_brightness)
> + if ((int)value > (int)asus->kbd_led.max_brightness)
> value = asus->kbd_led.max_brightness;
> - else if (value < 0)
> + else if ((int)value < 0)
> value = 0;
>
> asus->kbd_led_wk = value;
> @@ -656,6 +657,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
>
> asus->kbd_led_wk = led_val;
> asus->kbd_led.name = "asus::kbd_backlight";
> + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> asus->kbd_led.brightness_set = kbd_led_set;
> asus->kbd_led.brightness_get = kbd_led_get;
> asus->kbd_led.max_brightness = 3;
> @@ -1745,6 +1747,15 @@ static void asus_wmi_notify(u32 value, void *context)
> }
> }
>
> + if (code == NOTIFY_KBD_BRTUP) {
> + kbd_led_set(>kbd_led, asus->kbd_led_wk + 1);
> + goto exit;
> + }
> + if (code == NOTIFY_KBD_BRTDWN) {
> + kbd_led_set(>kbd_led, asus->kbd_led_wk - 1);
> + goto exit;
> + }
> +
> if (is_display_toggle(code) &&
> asus->driver->quirks->no_display_toggle)
> goto exit;
> --
> 2.11.0
>
Gentle ping. Any suggestions? The v2 is based on the last comment from
Hans, https://lkml.org/lkml/2018/6/5/270. Please let me know if nay. Thanks
Chris
Re: [PATCH v2 1/2] platform/x86: asus-wmi: Call led hw_changed API on kbd brightness change
On Mon, Jun 11, 2018 at 3:18 PM, Chris Chiu wrote:
> Make asus-wmi notify on hotkey kbd brightness changes, listen for
> brightness events and update the brightness directly in the driver.
> For this purpose, bound check on brightness in kbd_led_set must be
> based on the same data type to prevent illegal value been set.
>
> Update the brightness by led_classdev_notify_brightness_hw_changed.
> This will allow userspace to monitor (poll) for brightness changes
> on the LED without reporting via input keymapping.
>
> Signed-off-by: Chris Chiu
> ---
> drivers/platform/x86/asus-wmi.c | 15 +--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 1f6e68f0b646..a18484f4bae3 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -460,6 +460,7 @@ static void kbd_led_update(struct work_struct *work)
> ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
>
> asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
> + led_classdev_notify_brightness_hw_changed(>kbd_led,
> asus->kbd_led_wk);
> }
>
> static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
> @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev,
>
> asus = container_of(led_cdev, struct asus_wmi, kbd_led);
>
> - if (value > asus->kbd_led.max_brightness)
> + if ((int)value > (int)asus->kbd_led.max_brightness)
> value = asus->kbd_led.max_brightness;
> - else if (value < 0)
> + else if ((int)value < 0)
> value = 0;
>
> asus->kbd_led_wk = value;
> @@ -656,6 +657,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
>
> asus->kbd_led_wk = led_val;
> asus->kbd_led.name = "asus::kbd_backlight";
> + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> asus->kbd_led.brightness_set = kbd_led_set;
> asus->kbd_led.brightness_get = kbd_led_get;
> asus->kbd_led.max_brightness = 3;
> @@ -1745,6 +1747,15 @@ static void asus_wmi_notify(u32 value, void *context)
> }
> }
>
> + if (code == NOTIFY_KBD_BRTUP) {
> + kbd_led_set(>kbd_led, asus->kbd_led_wk + 1);
> + goto exit;
> + }
> + if (code == NOTIFY_KBD_BRTDWN) {
> + kbd_led_set(>kbd_led, asus->kbd_led_wk - 1);
> + goto exit;
> + }
> +
> if (is_display_toggle(code) &&
> asus->driver->quirks->no_display_toggle)
> goto exit;
> --
> 2.11.0
>
Gentle ping. Any suggestions? The v2 is based on the last comment from
Hans, https://lkml.org/lkml/2018/6/5/270. Please let me know if nay. Thanks
Chris
