Re: [PATCH v2 0/2] Fix SAS wildcard scan on smartpqi and other controllers

[Martin Wilck <mwilck@suse.com>]

Hi Martin, Ranjan, Don, Hannes, all,

I need some advice / guidance.

TL;DR:

- Does it make sense to pursue the idea of this patch by adding proper
locking and making some additional amendments?
- Otherwise, can we revert 37c4e72b0651 and find a different solution
for rescanning NVMe drives on mpt3sas and mpi3mr?

Long version:

Thanks for the link with the Gemini review. My current approach is
arguably too simplistic.

The question is, then, what to do about 37c4e72b0651 ("scsi: Fix
sas_user_scan() to handle wildcard and multi-channel scans"). As a
matter of fact, this commit introduces severe regressions (system
stall) at least for smartpqi and hisi_sas. 

Looking back at the discussion about this commit [2], it was submitted
to fix scanning of NVMe drives, which mpt3sas and mpi3mr may present as
SCSI targets in a certain mode of operation, in which these drivers use
the channel number to distinguish NVMe and SCSI devices from each
other. This use of the channel number is inconsistent with how other
SAS drivers use it. Therefore this commit broke SAS scanning for other
drivers. The obvious fix would  be to revert 37c4e72b0651. This is
actually what we (SUSE) have done in our downstream kernel so far.

This would mean that Broadcom would need to come up with a different
way to handle the scanning for mpt3sas and mpi3mr in "Tri-mode".

Other than that: I still believe that the general idea of using
scan_start() and scan_finished() not only for the initial host adapter
scan but also for rescan operations is reasonable. After all, a "dumb"
wildcard scan doesn't make a lot of sense for drivers "which find their
own targets", for which scan_start() and scan_finished were introduced
in 1aa8fab2acf1 ("[SCSI] Make scsi_scan_host work for drivers which
find their own targets").

But the issues pointed out by Gemini are real, and while I could fix
the locking, the problem that some drivers may reinitialize the
hardware in scan_start() is tricky. Some detailed replies on Gemini's
review are appended below.

Thanks,
Martin

[1] https://sashiko.dev/#/patchset/20260421202018.511388-1-mwilck%40suse.com
[2] https://lore.kernel.org/linux-scsi/CAFdVvOwjy+2ORJ6uJkspiLTPF05481U7gcS4QohFOFGPqAs8ig@mail.gmail.com/

On Thu, 2026-04-30 at 12:30 -0400, Martin K. Petersen wrote:
> 
> https://sashiko.dev/#/patchset/20260421202018.511388-1-mwilck%40suse.com


P.S: Detailed replies to Gemini's review:

> Does bypassing scsi_scan_host_selected() here skip necessary power
> management and serialization steps?
> 
> Typically, wildcard scans go through scsi_scan_host_selected(), which
> secures the host adapter via scsi_autopm_get_host(), checks
> scsi_host_scan_allowed(), and takes shost->scan_mutex.
>
> By calling do_scsi_scan_host() directly, it appears scan_start()
could be
> invoked while the host adapter is runtime-suspended, which might lead
to
> hardware faults.

The call to scsi_scan_host_selected() was introduced in commit
37c4e72b0651, which my patch is intended to fix; before that commit,
scan_mutex was not taken in the sas_user_scan() code path. It's still
explicitly not taken for the important case of scanning channel 0;
sas_user_scan() takes only sas_host->lock, which protects just the rphy
list.

So yes, sas_user_scan_with_scan_start() code path should acquire the
scan_mutex. I suppose that the mutex should be acquired in
sas_user_scan(), actually.

> Also, without holding scan_mutex and checking
scsi_host_scan_allowed(),
> is there a race condition if a sysfs scan is triggered concurrently
with
> driver removal? If scsi_remove_host() tears down the host
asynchronously,
> the while loop in do_scsi_scan_host() might poll shost->hostt-
>scan_finished()
> after the driver module has unloaded, potentially causing a use-
after-free.

True. Taking the scan_mutex should fix this.

> Similarly, multiple concurrent sysfs writes could execute
scan_start()
> simultaneously without scan_mutex protection. Can this corrupt
internal
> driver state?

Right. Again, this can be fixed by taking scan_mutex.

> Additionally, is it safe to repurpose scan_start() for runtime sysfs
> scans?
> 
> Historically, shost->hostt->scan_start() seems to be used exclusively
> for one-time asynchronous boot initialization in do_scan_async().
> 
> Many SAS drivers might implement this as a hardware initialization
routine
> that resets physical links.
> 
> If exposed to a runtime userspace sysfs trigger, could this
unintentionally
> bounce active SAS links, drop active I/O, or cause controller hangs
for
> drivers that assume scan_start() is only used during boot?

This is not the case for smartpqi, but other drivers (e.g. hisi_sas)
actually seem to enable PHYs in the scan_start() code path. Not sure
how this can be fixed in general. AFAICS it would require thorough
understanding of the hardware / firmware of all affected drivers, which
I don't have.

Would it make sense pass a flag to scan_start() indicating whether we
initiate a first-time scan or a rescan, and skip the hardware
initialization in the latter case?