Re: [PATCH v6 12/27] scsi-multipath: introduce scsi_device head structure

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The struct `mpath_head_template smpdt` is defined completely empty, lacking logically required callbacks, leading to NULL pointer dereferences.
- [Medium] The patch omits the logic to create sysfs links to member `scsi_device`'s, contradicting the stated commit message.
- [Low] The function `scsi_multipath_sysfs_group_visible` and the macro `DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(scsi_multipath_sysfs)` generate unused static code, causing a `-Wunused-function` warning.
--

--- Patch [12]: [PATCH v6 12/27] scsi-multipath: introduce scsi_device head structure ---
commit 41b0248171de9edac7171ac10282e0c5b3f1056c
Author: John Garry <[email protected]>

scsi-multipath: introduce scsi_device head structure

Introduce a scsi_mpath_head structure to manage multipathing for a
scsi_device. A list of scsi_mpath_head structures is managed to lookup for
matching multipathed scsi_device's.

> The sysfs device folder will have links to the scsi_device's so, it will
> be possible to lookup the member scsi_device's.

[Severity: Medium]
I notice the patch doesn't seem to contain the code to create these sysfs links
to the member scsi_device structures. Is this implementation missing, or is it
planned for a future patch?

diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
index 5559e2e8d64a8..30a2d01be8aaf 100644
--- a/drivers/scsi/scsi_multipath.c
+++ b/drivers/scsi/scsi_multipath.c

[ ... ]

> +static const struct attribute_group scsi_mpath_device_attrs_group = {
> +	.attrs = scsi_mpath_device_attrs,
> +};
> +
> +static bool scsi_multipath_sysfs_group_visible(struct kobject *kobj)
> +{
> +	return true;
> +}
> +DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(scsi_multipath_sysfs)

[Severity: Low]
Is this function and macro used anywhere in this patch? 

It looks like DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE generates a static function 
that isn't assigned to any attribute_group.is_visible callback, which might 
cause a unused-function compiler warning.

> +
> +static const struct attribute_group *scsi_mpath_device_groups[] = {
> +	&scsi_mpath_device_attrs_group,
> +	NULL
> +};

[ ... ]

>  static int scsi_multipath_sdev_init(struct scsi_device *sdev)
>  {
>  	struct Scsi_Host *shost = sdev->host;
> @@ -91,6 +144,74 @@ static int scsi_multipath_sdev_init(struct scsi_device *sdev)
>  	return 0;
>  }
>  
> +static struct mpath_head_template smpdt = {
> +};

[Severity: High]
Will this cause a null pointer dereference when the generic multipath core
attempts to evaluate I/O paths? 

Since smpdt is defined completely empty without any callback assignments, 
function pointers like mpath_head->mpdt->is_disabled will be null when 
dereferenced by the framework.

> +
> +static struct scsi_mpath_head *scsi_mpath_alloc_head(char *vpd_id)
> +{
> +	struct scsi_mpath_head *scsi_mpath_head;

[ ... ]

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