On 04/08/2020 10:51, Deepak.Ukey@xxxxxxxxxxxxx wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
On 04/08/2020 07:33,Deepak.Ukey@xxxxxxxxxxxxx wrote:
Hi Christoph,
Yes, It is better to be implemented in libsas. Since the out of box pm80xx driver has this support, we would like to push this for the time being. We will see how this can be moved to libsas.
Other libsas users may like this feature. And libsas does already support SATA spin-up hold events - as does pm8001 - but there's not really much to that in libsas.
Question: why have a module param to enable this feature? Why not solely rely on the seeprom spin-up interval, whereby a value of 0 means no staggered spin-up?
- Setting spin-up interval may increase the time for device discovery. Customer who has a valid spin up interval
Please use standard mailing list practices in replying.
- configured can still turn of this using module parameter. Or
else, they have to reflash the seeprom.
So another knob to control the driver. Anyway I think that it would be
good for libsas to support this feature to benefit all users.
All the tunables from the LLDD can be fed to libsas, and libsas just
needs to do what you do in pm8001_spinup_timedout() to callback to the
LLDD to enable the SPINUP.
BTW, out of interest, what is this change for:
> void pm8001_scan_start(struct Scsi_Host *shost)
> {
> int i;
> + unsigned long lock_flags;
> struct pm8001_hba_info *pm8001_ha;
> struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
> + DECLARE_COMPLETION(comp);
note: this should be the _ONSTACK() variant, and is it safe to even
reuse in the loop, below?
> pm8001_ha = sha->lldd_ha;
> /* SAS_RE_INITIALIZATION not available in SPCv/ve */
> if (pm8001_ha->chip_id == chip_8001)
> PM8001_CHIP_DISP->sas_re_init_req(pm8001_ha);
> - for (i = 0; i < pm8001_ha->chip->n_phy; ++i)
> - PM8001_CHIP_DISP->phy_start_req(pm8001_ha, i);
> +
> + if (pm8001_ha->pdev->device == 0x8001 ||
> + pm8001_ha->pdev->device == 0x8081 ||
> + (pm8001_ha->spinup_interval != 0)) {
> + for (i = 0; i < pm8001_ha->chip->n_phy; ++i)
> + PM8001_CHIP_DISP->phy_start_req(pm8001_ha, i);
> + } else {
> + for (i = 0; i < pm8001_ha->chip->n_phy; ++i) {
> + spin_lock_irqsave(&pm8001_ha->lock, lock_flags);
> + pm8001_ha->phy_started = i;
> + pm8001_ha->scan_completion = ∁
> + pm8001_ha->phystart_timedout = 1;
> + spin_unlock_irqrestore(&pm8001_ha->lock, lock_flags);
> + PM8001_CHIP_DISP->phy_start_req(pm8001_ha, i);
> + wait_for_completion_timeout(&comp,
> + msecs_to_jiffies(500));
> + if (pm8001_ha->phystart_timedout)
> + PM8001_MSG_DBG(pm8001_ha, pm8001_printk(
> + "Timeout happened for phyid = %d\n", i));
> + }
> + spin_lock_irqsave(&pm8001_ha->lock, lock_flags);
> + pm8001_ha->phy_started = -1;
> + pm8001_ha->scan_completion = NULL;
> + spin_unlock_irqrestore(&pm8001_ha->lock, lock_flags);
> + }
> }
>
Thanks,
John
