On Tue, May 03, 2022 at 10:06:43PM +0200, Hannes Reinecke wrote:
> @@ -373,9 +373,11 @@ static int zfcp_scsi_eh_target_reset_handler(struct scsi_cmnd *scpnt)
>
> static int zfcp_scsi_eh_host_reset_handler(struct scsi_cmnd *scpnt)
> {
> - struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(scpnt->device);
> - struct zfcp_adapter *adapter = zfcp_sdev->port->adapter;
> - int ret = SUCCESS, fc_ret;
> + struct Scsi_Host *host = scpnt->device->host;
> + struct zfcp_adapter *adapter = (struct zfcp_adapter *)host->hostdata[0];
> + int ret = SUCCESS;
> + unsigned long flags;
> + struct zfcp_port *port;
>
> if (!(adapter->connection_features & FSF_FEATURE_NPIV_MODE)) {
> zfcp_erp_port_forced_reopen_all(adapter, 0, "schrh_p");
> @@ -383,9 +385,16 @@ static int zfcp_scsi_eh_host_reset_handler(struct scsi_cmnd *scpnt)
> }
> zfcp_erp_adapter_reopen(adapter, 0, "schrh_1");
> zfcp_erp_wait(adapter);
> - fc_ret = fc_block_scsi_eh(scpnt);
> - if (fc_ret)
> - ret = fc_ret;
> +
> + spin_lock_irqsave(&adapter->port_list_lock, flags);
That doesn't compile:
|| In file included from ./include/linux/kref.h:16,
|| drivers/s390/scsi/zfcp_scsi.c: In function ‘zfcp_scsi_eh_host_reset_handler’:
drivers/s390/scsi/zfcp_scsi.c|389 col 27| error: passing argument 1 of ‘spinlock_check’ from incompatible pointer type [-Werror=incompatible-pointer-types]
|| 389 | spin_lock_irqsave(&adapter->port_list_lock, flags);
|| | ^~~~~~~~~~~~~~~~~~~~~~~~
|| | |
|| | rwlock_t *
You probably want `read_lock_irqsave()`.
The locking order looks fine, we already have places where we take the
`port_list_lock`, and then the `host_lock` (ffr:
`zfcp_erp_action_dismiss_adapter()`).
> + list_for_each_entry(port, &adapter->port_list, list) {
> + struct fc_rport *rport = port->rport;
Like Steffen said in the other review, this may be NULL here, since
`scpnt` can be from a different context, and there is no guarantees this
is always assigned, if you iterate over all port of an adapter.
So you want:
if (!rport)
continue;
Or some such, with a similar effect.
> +
> + ret = fc_block_rport(rport);
> + if (ret)
> + break;
> + }
> + spin_unlock_irqrestore(&adapter->port_list_lock, flags);
Otherwise I like this better than having the code open-coded here, also
don't see any other obvious things missing than the two above.
>
> zfcp_dbf_scsi_eh("schrh_r", adapter, ~0, ret);
> return ret;
--
Best Regards, Benjamin Block / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH / https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen / Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294
