Re: [PATCH v5 1/3] serdev: handle devices without an ACPI or device tree node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] serdev: unupdated serdev_device_add() callers break build in bisect
- [Medium] serdev: missing modpost wiring for struct serdev_device_id
--

commit 5b1ae2343b2c9133fe8d3921a65c4b7fe749f748
Author: ChunAn Wu <an.wu@canonical.com>

serdev: handle devices without an ACPI or device tree node

This commit adds modalias and id_table support to the serdev bus so that
serdev clients without an ACPI companion or device tree node can emit
modalias uevents and match against struct serdev_device_id tables. It also
introduces include/linux/device-id/serdev.h and updates serdev_device_add()
to accept a modalias string.

> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 7500efcdfc218..e0517d5a48da2 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
[ ... ]
> @@ -86,28 +101,57 @@ static const struct device_type serdev_ctrl_type = {
[ ... ]
>  /**
>   * serdev_device_add() - add a device previously constructed via serdev_device_alloc()
>   * @serdev:	serdev_device to be added
> + * @modalias:	Driver name or alias, used to match devices which have neither
> + *		an ACPI companion nor a device tree node. NULL for devices
> + *		enumerated from firmware.
>   */
> -int serdev_device_add(struct serdev_device *serdev)
> +int serdev_device_add(struct serdev_device *serdev, const char *modalias)
>  {
>  	struct serdev_controller *ctrl = serdev->ctrl;
>  	struct device *parent = serdev->dev.parent;
>  	int err;
>  
> +	if (modalias)
> +		strscpy(serdev->modalias, modalias);

[Severity: Medium]
Does changing the signature of serdev_device_add() here without updating all
in-tree callers break compilation at this commit during a git bisect when
CONFIG_DELL_UART_BACKLIGHT or CONFIG_YT2_1380 is enabled?

While this commit updates the callers in drivers/tty/serdev/core.c,
drivers/platform/x86/x86-android-tablets/core.c, and
drivers/power/sequencing/pwrseq-pcie-m2.c to pass NULL, two callers are left
passing a single argument at this point in the series:

drivers/platform/x86/dell/dell-uart-backlight.c:
dell_uart_bl_pdev_probe()
	ret = serdev_device_add(serdev);

drivers/platform/x86/lenovo/yoga-tab2-pro-1380-fastcharger.c:
yt2_1380_fc_pdev_probe()
	ret = serdev_device_add(serdev);

Although the subsequent commits in the series ("platform/x86:
dell-uart-backlight: Bind through normal serdev matching" and "platform/x86:
lenovo-yoga-tab2-pro-1380-fastcharger: Bind through normal serdev matching")
update those two drivers to pass KBUILD_MODNAME, should both callers be
updated to pass NULL in this commit so that every patch in the series builds
cleanly?

[ ... ]
> diff --git a/include/linux/device-id/serdev.h b/include/linux/device-id/serdev.h
> new file mode 100644
> index 0000000000000..96644953ceced
> --- /dev/null
> +++ b/include/linux/device-id/serdev.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef LINUX_DEVICE_ID_SERDEV_H
> +#define LINUX_DEVICE_ID_SERDEV_H
> +
> +#ifdef __KERNEL__
> +typedef unsigned long kernel_ulong_t;
> +#endif
> +
> +/* serdev */
> +
> +#define SERDEV_NAME_SIZE	40
> +#define SERDEV_MODULE_PREFIX	"serial:"
> +
> +struct serdev_device_id {
> +	char name[SERDEV_NAME_SIZE];
> +	kernel_ulong_t driver_data;	/* Data private to the driver */
> +};
> +
> +#endif /* ifndef LINUX_DEVICE_ID_SERDEV_H */

[Severity: Medium]
Should struct serdev_device_id also be wired into modpost so that
MODULE_DEVICE_TABLE(serdev, ...) generates serial:* module aliases?

In drivers/tty/serdev/core.c, serdev_device_uevent() emits
MODALIAS=serial:<modalias> using SERDEV_MODULE_PREFIX:

	return add_uevent_var(env, "MODALIAS=%s%s", SERDEV_MODULE_PREFIX,
			      serdev->modalias);

However, include/linux/mod_devicetable.h does not include
device-id/serdev.h, scripts/mod/devicetable-offsets.c does not define
DEVID(serdev_device_id) and DEVID_FIELD(serdev_device_id, name), and
devtable[] in scripts/mod/file2alias.c has no "serdev" entry or handler. As
a result, handle_moddevtable() silently ignores MODULE_DEVICE_TABLE(serdev,
...) declarations, preventing uevent-based module autoloading for modular
serdev client drivers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792831564.git.an.wu@canonical.com?part=1