Re: [PATCH RFC v3 2/7] ata: libata-scsi: Add ata_internal_queuecommand()

[Hannes Reinecke]

On 10/27/22 11:56, John Garry wrote:
> On 27/10/2022 02:45, Damien Le Moal wrote:
> > On 10/25/22 19:32, John Garry wrote:
> > > Add callback to queue reserved commands - call it "internal" as this is
> > > what libata uses.
> > >
> > > Also add it to the base ATA SHT.
> > >
> > > Signed-off-by: John Garry <john.garry@xxxxxxxxxx>
> > > ---
> > >   drivers/ata/libata-scsi.c | 14 ++++++++++++++
> > >   include/linux/libata.h    |  5 ++++-
> > >   2 files changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > > index 30d7c90b0c35..0d6f37d80137 100644
> > > --- a/drivers/ata/libata-scsi.c
> > > +++ b/drivers/ata/libata-scsi.c
> > > @@ -1118,6 +1118,20 @@ int ata_scsi_dev_config(struct scsi_device 
> > > *sdev, struct ata_device *dev)
> > >       return 0;
> > >   }
> > > +int ata_internal_queuecommand(struct Scsi_Host *shost, struct 
> > > scsi_cmnd *scmd)
> > > +{
> > > +    struct ata_port *ap;
> > > +    int res;
> > > +
> > > +    ap = ata_shost_to_port(shost);
> >
> > You can have this initialization together with the ap declaration.
> >
> > > +    spin_lock_irq(ap->lock);
> > > +    res = ata_sas_queuecmd(scmd, ap);
> > > +    spin_unlock_irq(ap->lock);
> > > +
> > > +    return res;
> > > +}
> > > +EXPORT_SYMBOL_GPL(ata_internal_queuecommand);
> >
> > I am officially lost here. Do not see why this function is needed...
>
> The general idea in this series is to send ATA internal commands as 
> requests. And this function is used as the SCSI midlayer to queue 
> reserved commands. See how it is plugged into __ATA_BASE_SHT, below.
>
> So we have this overall flow:
>
> ata_exec_internal_sg():
>   -> alloc request
>   -> blk_execute_rq_nowait()
>       ... -> scsi_queue_rq()
>          -> sht->reserved_queuecommd()
>              -> ata_internal_queuecommand()
>
> And then we have ata_internal_queuecommand() -> ata_sas_queuecmd() -> 
> ata_scsi_queue_internal() -> ata_qc_issue().
>
> Hope it makes sense.
>
> Thanks,
> John
>
> > > +
> > >   /**
> > >    *    ata_scsi_slave_config - Set SCSI device attributes
> > >    *    @sdev: SCSI device to examine
> > > diff --git a/include/linux/libata.h b/include/linux/libata.h
> > > index 8938b584520f..f09c5dca16ce 100644
> > > --- a/include/linux/libata.h
> > > +++ b/include/linux/libata.h
> > > @@ -1141,6 +1141,8 @@ extern int ata_std_bios_param(struct 
> > > scsi_device *sdev,
> > >                     sector_t capacity, int geom[]);
> > >   extern void ata_scsi_unlock_native_capacity(struct scsi_device *sdev);
> > >   extern int ata_scsi_slave_config(struct scsi_device *sdev);
> > > +extern int ata_internal_queuecommand(struct Scsi_Host *shost,
> > > +                struct scsi_cmnd *scmd);
> > >   extern void ata_scsi_slave_destroy(struct scsi_device *sdev);
> > >   extern int ata_scsi_change_queue_depth(struct scsi_device *sdev,
> > >                          int queue_depth);
> > > @@ -1391,7 +1393,8 @@ extern const struct attribute_group 
> > > *ata_common_sdev_groups[];
> > >       .slave_destroy        = ata_scsi_slave_destroy,    \
> > >       .bios_param        = ata_std_bios_param,        \
> > >       .unlock_native_capacity    = ata_scsi_unlock_native_capacity,\
> > > -    .max_sectors        = ATA_MAX_SECTORS_LBA48
> > > +    .max_sectors        = ATA_MAX_SECTORS_LBA48,\
> > > +    .reserved_queuecommand = ata_internal_queuecommand
> > >   #define ATA_SUBBASE_SHT(drv_name)                \
> > >       __ATA_BASE_SHT(drv_name),                \

But that means we can't use it before the SCSI host is initialized; some 
HBAs require to send commands before the host can be initialized properly.

Cheers,

Hannes
--
Dr. Hannes Reinecke                Kernel Storage Architect
hare@xxxxxxx                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman