Re: [PATCH v3 1/3] hwmon: (asus_rog_ryujin) Add per-device configuration

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The HID hardware and input streams are started before driver synchronization primitives are initialized, creating a race condition that causes a NULL pointer dereference on device plug.
--

--- Patch [1]: [PATCH v3 1/3] hwmon: (asus_rog_ryujin) Add per-device configuration ---
commit 5da50001b696b978e4b3741acfab6782c708ed93
Author: Arie Miller <[email protected]>

hwmon: (asus_rog_ryujin) Add per-device configuration

Move model-specific report offsets and capabilities into a device
information structure. This prepares the driver for coolers which use
a different report layout or do not include the external fan controller.

Link: https://lore.kernel.org/r/[email protected]

> diff --git a/drivers/hwmon/asus_rog_ryujin.c b/drivers/hwmon/asus_rog_ryujin.c
> index 10a1f5aca988d..80563ce38519d 100644
> --- a/drivers/hwmon/asus_rog_ryujin.c
> +++ b/drivers/hwmon/asus_rog_ryujin.c

[ ... ]

> @@ -471,11 +495,15 @@ static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id
>  	struct rog_ryujin_data *priv;
>  	int ret;
>  
> +	if (!id->driver_data)
> +		return -EINVAL;
> +
>  	priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
>  	priv->hdev = hdev;
> +	priv->info = (const struct rog_ryujin_device_info *)id->driver_data;
>  	hid_set_drvdata(hdev, priv);

[Severity: High]
This is a pre-existing issue, but can the initialization order in this
probe function cause a race condition leading to a NULL pointer dereference?

Further down in rog_ryujin_probe(), the driver starts the hardware and opens
the stream before initializing the completion structures:

        ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
        ...
        ret = hid_hw_open(hdev);
        ...
        init_completion(&priv->cooler_status_received);

If the device sends a status report immediately after hid_hw_open() is called,
rog_ryujin_raw_event() will be invoked to process it. This triggers an attempt
to use the uninitialized completion structure:

drivers/hwmon/asus_rog_ryujin.c:rog_ryujin_raw_event() {
        ...
        if (!completion_done(&priv->cooler_status_received))
            complete_all(&priv->cooler_status_received);
        ...
}

Because the priv structure was allocated with devm_kzalloc(), the
swait_queue_head inside the completion is initialized to all zeroes.

When complete_all() iterates over the zeroed wait queue, its task_list.next
pointer is NULL, which would cause an immediate kernel panic.

Should the init_completion() calls be moved up before hid_hw_start() to
ensure they are ready before any reports can arrive?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1