Re: [PATCH 00/10] fujitsu-laptop: renames and cleanups

From: Andy Shevchenko
Date: Wed Feb 08 2017 - 10:25:02 EST

On Wed, Feb 8, 2017 at 3:46 PM, MichaÅ KÄpieÅ <kernel@xxxxxxxxxx> wrote:
> This series of patches was originally submitted by Alan Jenkins in
> September 2009. For various reasons they were never acted upon before.
> Sadly, their original state makes them unreviewable due to multiple
> changes happening within one patch and unreliable commit messages.
> In order for Alan's efforts not to go to waste, I have rebased his
> patches on top of dvhart/for-next, further split them into logically
> separate changes and rewrote commit messages from scratch, based on the
> assumed intent of the original author.
> Here is how the original patches map to the rebased and split patches:
> 1/4 -> 1-6/10
> 2/4 -> 7-8/10
> 3/4 -> 9/10
> 4/4 -> 10/10
> Some of these patches raise checkpatch warnings. This is intentional,
> in order to make renames clean and easy to review. All of these issues
> will be fixed by upcoming patch series.
> Not all ideas suggested by Alan are present in the rebased series. A
> brief discussion of each rejected suggestion follows (most important
> issues first).
> - Restructure fujitsu_init(). Quite frankly, I am pretty sure Alan
> did not test his series on Fujitsu hardware with a logo lamp and/or
> keyboard lamps. Neither have I, but with his original patches
> applied, what happens on such a model is:
> 1. fujitsu_init() calls acpi_bus_register_driver().
> 2. acpi_fujitsu_laptop_add() is called.
> 3. fujitsu_laptop is kzalloc()'ed.
> 4. fujitsu_laptop->pf_device->dev is accessed.
> This results in a NULL dereference, because the platform device is
> only allocated and registered later in fujitsu_init().
> On the other hand, registering the platform device before the ACPI
> driver is also incorrect due to reasons I already pointed out in
> another thread (in short: we cannot _assume_ FUJ02E3 is present).
> This can only be fixed properly by registering the platform device
> inside acpi_fujitsu_laptop_add(), but I am still waiting for
> comments from Darren and Andy in the other thread before moving
> forward down this path.
> - Bail out when FUJ02B1 is not present. It could have been deemed
> correct back in the day, but we now know that Fujitsu started
> shipping devices without that ACPI device present, though with
> FUJ02E3 still in place. These two ACPI devices are independent and
> thus should not rely on each other's presence.
> - Move keycode[1-5] fields to struct fujitsu_laptop. Doing this
> causes ordering issues inside fujitsu_init(), while a patch series I
> have queued that makes fujitsu-laptop use a sparse keymap removes
> these fields altogether.
> - Allocate and free fujitsu_bl and fujitsu_laptop inside ACPI
> callbacks. While elegant and correct, this also causes ordering
> issues inside fujitsu_init() and can only be done once platform
> device registration is properly fixed.
> - Sync backlight power status in acpi_fujitsu_bl_add(). The long-term
> objective for fujitsu-laptop should be to achieve a clean split
> between the backlight-related part and the laptop-related part.
> This change keeps both parts intertwined. My fujitsu_init() cleanup
> series contains a similar fix, but I have since found a different
> solution to this problem which I will post once Alan's rebased
> series gets applied.
> - Remove dmi_check_cb_common(). The sparse keymap series I have
> queued gets rid of this function without introducing an additional
> field inside struct fujitsu_laptop.
> Moreover, some other minor changes present in original patch 1/4 were
> left out. A brief discussion of each such case follows.
> Notify code 0x80 is used by both parts of the driver (the
> brightness-related one and the laptop-related one) and thus it should
> not be annotated using the "_BL" infix specific to brightness-related
> code.
> ...
> -#endif
> This does not really bring any benefit while breaking consistency with
> other parts of the driver.
> - dmi_check_system(fujitsu_laptop_dmi_table);
> + dmi_check_system(fujitsu_dmi_table);
> The original patches moved this dmi_check_system() call to
> acpi_fujitsu_laptop_add(), which justifies renaming fujitsu_dmi_table to
> fujitsu_laptop_dmi_table. However, for reasons discussed above, the
> rebased patches leave the dmi_check_system() call inside fujitsu_init(),
> thus making the rename dubious.
> There are multiple changes like this in original patch 1/4. They are
> obviously wrong and were probably introduced by an unreviewed automatic
> replacement.
> @@ -495,7 +495,6 @@ static ssize_t
> show_max_brightness(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> -
> int ret;
> There are four instances of such a removal in original patch 1/4. They
> are obviously correct, though they all happen inside functions related
> to platform device attributes which I hope will soon get removed
> altogether. Thus I refrained from applying these to reduce churn (at
> least a bit).
> Finally, some of the changes suggested by Alan were already applied
> along the way:
> - kmalloc() + memset() occurences were changed to kzalloc() by commit
> 6c75dd0f965b ("drivers/platform/x86: Use kzalloc").
> - Unused debug macros were removed by commit 00816e1b3839
> ("fujitsu-laptop: Remove unused macros").

Nice clean up!
So, I would apply 1-7, for the rest I need more time to review.

Regarding ACPI case and device presents you may assume it if you just call
acpi_walk_namespace() (AFAIU) and check _STA for the device if it's in
the table.

So, at any point you may have got understanding if device is present
or not, and if it's active or not.

With Best Regards,
Andy Shevchenko