Re: [PATCH] scsi: isci: initialize shost fully before calling scsi_add_host()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/01/2019 02:34, Martin K. Petersen wrote:

John,

So how about just drop these APIs and let the user set the shost
protection parameters directly, like other shost parameters,

The protection interfaces here obviously predate the block layer
allocation changes that made this particular issue pop up.

which should make it a bit clearer when these should be set,
i.e. before scsi_add_host()?

In general, I am not so keen on the somewhat messy intersection between
the host parameters and the host template. The static host templates
made lots of sense in the days of Seagate ST01 and fixed hardware
capabilities.  But reality is that most modern controllers have to query
firmware interfaces to figure out what their actual capabilities are.

Hi Martin,

I am not suggested setting the parameters via scsi host template, but rather dynamically (as we currently do) but just drop the set helper functions, like:

    shost->max_channel = 1;
    shost->max_cmd_len = 16;

    ...

    if (hisi_hba->prot_mask) {
        dev_info(dev, "Registering for DIF/DIX prot_mask=0x%x\n",
             prot_mask);
-        scsi_host_set_prot(hisi_hba->shost, prot_mask);
+        shost->prot_capabilities = prot_mask;
    }

    rc = scsi_add_host(shost, dev);
    if (rc)
        goto err_out_ha;

    rc = sas_register_ha(sha);
    if (rc)
        goto err_out_register_ha;

I find that it is not crystal clear when scsi_host_set_prot() and scsi_host_set_guard() should be called, but not so for setting the shost parameters directly, which is common.


So in this case I think that accessor functions are actually better
because they allow us to print a big fat warning when you twiddle
something you shouldn't post-initialization. So that's something I think
we could--and should--improve.


Sure, this is an alternative, but I would rather make it obvious when these parameters should be set so that this would not be required.

Thanks,
John





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux