Re: [PATCH v4 2/2] scsi: leapraid: Add driver documentation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Custom sysfs attributes introduced for standard SCSI/SAS features.
- [Medium] Kernel-space SMART polling thread violates separation of concerns.
- [Medium] Custom module parameters introduced for interrupt routing management.
--

commit 60c41a7c6ea7e2df6e1711553263bb526e40d738
Author: Dongdong Hao <doubled@leap-io-kernel.com>

scsi: leapraid: Add driver documentation

This patch adds the necessary documentation for the LeapRAID SCSI driver
to the kernel's documentation tree.

> diff --git a/Documentation/scsi/leapraid.rst b/Documentation/scsi/leapraid.rst
> new file mode 100644
> index 0000000000000..4067f07e64283
> --- /dev/null
> +++ b/Documentation/scsi/leapraid.rst
[ ... ]
> +LeapRAID specific host attributes
> +=================================
> +
> +::
> +
> +   /sys/class/scsi_host/host*/fw_queue_depth
> +   /sys/class/scsi_host/host*/host_sas_address
> +   /sys/class/scsi_host/host*/board_name
> +
> +The host "fw_queue_depth" read-only attribute shows the firmware queue
> +depth of the host.
> +
> +The host "host_sas_address" read-only attribute shows the SAS address
> +of the host.
> +
> +The host "board_name" read-only attribute shows the board name reported
> +by manufacturing page 0.
> +
> +LeapRAID specific disk attributes
> +=================================
> +
> +::
> +
> +   /sys/class/scsi_disk/host:bus:target:lun/device/sas_address
> +   /sys/class/scsi_disk/host:bus:target:lun/device/sas_device_handle
> +   /sys/class/scsi_disk/host:bus:target:lun/device/sas_ncq
> +
> +The disk "sas_address" read-only attribute shows the SAS address of the
> +disk.
> +
> +The read-only attribute "sas_device_handle" represents the disk's device
> +handle, which is a unique identifier maintained by the firmware.
> +
> +This attribute "sas_ncq" controls the Native Command Queuing (NCQ) feature
> +for SATA devices. A value of 0 indicates that NCQ is currently disabled or
> +not supported. Writing 1 attempts to enable NCQ on the device. If the
> +operation succeeds, the value remains 1, indicating that NCQ has been
> +successfully enabled.

[Severity: Medium]
Should these features be exposed via custom driver-specific sysfs nodes rather
than using standard kernel mechanisms?

In Linux, SAS topologies and addresses should generally be exposed via the
standard scsi_transport_sas class, and NCQ is typically managed through the
standard SCSI queue_depth interface. Introducing driver-specific sysfs nodes for
standard capabilities creates UAPI fragmentation and breaks compatibility with
standard userspace tools.

[ ... ]
> +3. msix_disable
> +---------------
> +
> +This parameter specifies whether MSI-X interrupts should be disabled. By
> +default, 0 keeps MSI-X enabled, providing high-performance interrupt
> +handling. Setting this parameter to 1 disables MSI-X, causing the driver
> +to fall back to legacy INTx interrupts.
> +
> +4. max_msix_vectors
> +-------------------
> +
> +This parameter sets the upper limit on the number of MSI-X interrupt
> +vectors that the driver will request during initialization. The default
> +value of -1 allows the driver to use all available vectors as provided
> +by the device. Setting a positive integer restricts the number of vectors.
> +
> +5. interrupt_mode
> +-----------------
> +
> +This parameter defines which interrupt delivery mechanism is used by the
> +driver. A value of 0 selects MSI-X mode (default), providing the highest
> +performance and scalability. A value of 1 switches to MSI mode, while 2
> +forces the driver to use legacy INTx interrupts.

[Severity: Medium]
Is it appropriate to expose these low-level interrupt routing knobs to users via
module parameters?

The Linux PCI core (pci_alloc_irq_vectors()) is designed to automatically handle
vector allocation and gracefully fall back from MSI-X to MSI to INTx. If there
are hardware quirks or broken MSI-X implementations, shouldn't they be handled
programmatically via standard PCI quirks rather than shifting the configuration
burden to users?

[ ... ]
> +7. smart_poll
> +-------------
> +
> +This parameter controls the SMART polling mechanism for SATA drives. When
> +enabled (1), the driver periodically checks the health of connected SATA
> +devices, allowing early detection of potential drive failures. The default
> +value is 0, which disables SMART polling.

[Severity: Medium]
Does this background SMART polling belong in the kernel driver?

SMART monitoring and health polling are typically userspace policy domains
handled by tools like smartd via standard ATA pass-through. Implementing this in
the kernel duplicates userspace functionality, unnecessarily bloats the kernel,
and could lead to concurrent ATA command collisions with standard monitoring
tools.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782110784.git.doubled@leap-io-kernel.com?part=2