Re: [PATCH v3 01/17] scsi-multipath: introduce basic SCSI device support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] The module parameter parsing logic loosely matches string prefixes instead of exact values, mistakenly accepting unintended input.
- [Low] The Kconfig symbol SCSI_MULTIPATH selects a non-existent symbol LIBMULTIPATH.
- [Low] The patch includes a non-existent header <linux/multipath.h> and references undefined structures, unconditionally breaking the build.
- [Low] The debug log message incorrectly states that 'IMPLICIT TPGS' is required, while the code accepts any ALUA TPGS support.
--

commit ad9f5e786299d2bbf9325dfe336051204d3f7369
Author: John Garry <john.g.garry@oracle.com>

scsi-multipath: introduce basic SCSI device support
    
For a scsi_device to support multipath, introduce structure
scsi_mpath_device to hold multipath-specific details.

> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index c3042393af234..d6c31df454825 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
[ ... ]
> @@ -76,6 +76,16 @@ config SCSI_LIB_KUNIT_TEST
>  
>  	  If unsure say N.
>  
> +config SCSI_MULTIPATH
> +	bool "SCSI multipath support (EXPERIMENTAL)"
> +	depends on SCSI_MOD
> +	select LIBMULTIPATH

[Severity: Low]
Does the config symbol LIBMULTIPATH exist in the tree? Selecting a
non-existent symbol causes unmet dependency warnings and breaks the
dependency inheritance chain.

> +	help
> +	  This option enables support for native SCSI multipath support for
> +	  SCSI host.
> +
> +	  If unsure say N.
> +
>  comment "SCSI support type (disk, tape, CD-ROM)"
>  	depends on SCSI

[ ... ]

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> new file mode 100644
> index 0000000000000..ff37cfdf2f9d1
> --- /dev/null
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -0,0 +1,146 @@
[ ... ]
> +static int scsi_multipath_param_set(const char *val, const struct kernel_param *kp)
> +{
> +	if (!val)
> +		return -EINVAL;
> +	if (!strncmp(val, "on", 2))
> +		scsi_multipath = SCSI_MULTIPATH_ON;
> +	else if (!strncmp(val, "always", 6))
> +		scsi_multipath = SCSI_MULTIPATH_ALWAYS;
> +	else if (!strncmp(val, "off", 3))
> +		scsi_multipath = SCSI_MULTIPATH_OFF;

[Severity: Medium]
Since scsi_multipath_param_set() uses strncmp() with exact lengths, won't it
silently accept strings like "once" or "office" instead of returning -EINVAL?
Should this use sysfs_streq() or strcmp() to ensure exact matches?

> +	else
> +		return -EINVAL;
> +
> +	return 0;
> +}

[ ... ]

> +int scsi_mpath_dev_alloc(struct scsi_device *sdev)
> +{
> +	int ret;
> +
> +	if (scsi_multipath == SCSI_MULTIPATH_OFF)
> +		return 0;
> +
> +	if (!scsi_device_tpgs(sdev) && (scsi_multipath != SCSI_MULTIPATH_ALWAYS)) {
> +		sdev_printk(KERN_DEBUG, sdev, "IMPLICIT TPGS are required for multipath support\n");

[Severity: Low]
This isn't a bug, but the log message says IMPLICIT TPGS is required, while
the check for scsi_device_tpgs(sdev) accepts any TPGS support (implicit,
explicit, or both). Could the message be updated to match the check?

> +		return 0;
> +	}
> +
> +	ret = scsi_multipath_sdev_init(sdev);
> +	if (ret)
> +		return ret;

[ ... ]

> diff --git a/include/scsi/scsi_multipath.h b/include/scsi/scsi_multipath.h
> new file mode 100644
> index 0000000000000..d3d410dafd17a
> --- /dev/null
> +++ b/include/scsi/scsi_multipath.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _SCSI_SCSI_MULTIPATH_H
> +#define _SCSI_SCSI_MULTIPATH_H
> +
> +#include <linux/list.h>
> +#include <linux/types.h>
> +#include <linux/rcupdate.h>
> +#include <linux/workqueue.h>
> +#include <linux/mutex.h>
> +#include <linux/blk-mq.h>
> +#include <linux/multipath.h>

[Severity: Low]
Does linux/multipath.h exist in the tree? Including a non-existent header
will unconditionally break the build for any file including this header.

> +#include <scsi/scsi.h>
> +#include <scsi/scsi_cmnd.h>
> +#include <scsi/scsi_dbg.h>
> +#include <scsi/scsi_device.h>
> +#include <scsi/scsi_devinfo.h>
> +#include <scsi/scsi_driver.h>
> +
> +#ifdef CONFIG_SCSI_MULTIPATH
> +#define SCSI_MPATH_DEVICE_ID_LEN 256
> +
> +struct scsi_mpath_device {
> +	struct mpath_device	mpath_device;

[Severity: Low]
Because linux/multipath.h is missing, struct mpath_device is undefined,
which results in an incomplete type error when CONFIG_SCSI_MULTIPATH is
enabled. Is there a missing patch series this depends on?

> +	struct scsi_device 	*sdev;
> +
> +	char			device_id_str[SCSI_MPATH_DEVICE_ID_LEN];
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703103402.3725011-1-john.g.garry@oracle.com?part=1