| Message ID | 0f6f2337e98f8a8a7dfae816bc001af28fa3a7be.1628231581.git.kwmad.kim@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: ufs: introduce vendor isr | expand |
On 8/5/21 11:34 PM, Kiwoong Kim wrote: > This patch is to activate some interrupt sources > that aren't defined in UFSHCI specifications. Those > purpose could be error handling, workaround or whatever. > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> > --- > drivers/scsi/ufs/ufshcd.c | 10 ++++++++++ > drivers/scsi/ufs/ufshcd.h | 8 ++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 05495c34a2b7..f85a9b335e0b 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -6428,6 +6428,16 @@ static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba) > static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status) > { > irqreturn_t retval = IRQ_NONE; > + int res = 0; > + unsigned long flags; > + > + retval = ufshcd_vops_intr(hba, &res); > + if (res) { > + spin_lock_irqsave(hba->host->host_lock, flags); > + hba->force_reset = true; > + ufshcd_schedule_eh_work(hba); > + spin_unlock_irqrestore(hba->host->host_lock, flags); > + } How can a non-standard extension have error handling code in common code? Please move the code under if (res) into the vendor code. > if (intr_status & UFSHCD_UIC_MASK) > retval |= ufshcd_uic_cmd_compl(hba, intr_status); > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index 971cfabc4a1e..1ed0a911f864 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -356,6 +356,7 @@ struct ufs_hba_variant_ops { > const union ufs_crypto_cfg_entry *cfg, int slot); > void (*event_notify)(struct ufs_hba *hba, > enum ufs_event_type evt, void *data); > + irqreturn_t (*intr)(struct ufs_hba *hba, int *res); > }; > > /* clock gating state */ > @@ -1296,6 +1297,13 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba, > hba->vops->config_scaling_param(hba, profile, data); > } > > +static inline irqreturn_t ufshcd_vops_intr(struct ufs_hba *hba, int *res) > +{ > + if (hba->vops && hba->vops->intr) > + return hba->vops->intr(hba, res); > + return IRQ_NONE; > +} > + > extern struct ufs_pm_lvl_states ufs_pm_lvl_states[]; So this code adds an indirect function call in the interrupt handler? This will have a negative impact on performance, especially on a kernel with security mitigations enabled. See also https://lwn.net/Articles/774743/. Thanks, Bart.
> On 8/5/21 11:34 PM, Kiwoong Kim wrote: > > This patch is to activate some interrupt sources that aren't defined > > in UFSHCI specifications. Those purpose could be error handling, > > workaround or whatever. > > > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> > > --- > > drivers/scsi/ufs/ufshcd.c | 10 ++++++++++ > > drivers/scsi/ufs/ufshcd.h | 8 ++++++++ > > 2 files changed, 18 insertions(+) > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > > index 05495c34a2b7..f85a9b335e0b 100644 > > --- a/drivers/scsi/ufs/ufshcd.c > > +++ b/drivers/scsi/ufs/ufshcd.c > > @@ -6428,6 +6428,16 @@ static irqreturn_t ufshcd_tmc_handler(struct > ufs_hba *hba) > > static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status) > > { > > irqreturn_t retval = IRQ_NONE; > > + int res = 0; > > + unsigned long flags; > > + > > + retval = ufshcd_vops_intr(hba, &res); > > + if (res) { > > + spin_lock_irqsave(hba->host->host_lock, flags); > > + hba->force_reset = true; > > + ufshcd_schedule_eh_work(hba); > > + spin_unlock_irqrestore(hba->host->host_lock, flags); > > + } > > How can a non-standard extension have error handling code in common code? > Please move the code under if (res) into the vendor code. Got it. > > > if (intr_status & UFSHCD_UIC_MASK) > > retval |= ufshcd_uic_cmd_compl(hba, intr_status); diff --git > > a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index > > 971cfabc4a1e..1ed0a911f864 100644 > > --- a/drivers/scsi/ufs/ufshcd.h > > +++ b/drivers/scsi/ufs/ufshcd.h > > @@ -356,6 +356,7 @@ struct ufs_hba_variant_ops { > > const union ufs_crypto_cfg_entry *cfg, int slot); > > void (*event_notify)(struct ufs_hba *hba, > > enum ufs_event_type evt, void *data); > > + irqreturn_t (*intr)(struct ufs_hba *hba, int *res); > > }; > > > > /* clock gating state */ > > @@ -1296,6 +1297,13 @@ static inline void > ufshcd_vops_config_scaling_param(struct ufs_hba *hba, > > hba->vops->config_scaling_param(hba, profile, data); > > } > > > > +static inline irqreturn_t ufshcd_vops_intr(struct ufs_hba *hba, int > > +*res) { > > + if (hba->vops && hba->vops->intr) > > + return hba->vops->intr(hba, res); > > + return IRQ_NONE; > > +} > > + > > extern struct ufs_pm_lvl_states ufs_pm_lvl_states[]; > > So this code adds an indirect function call in the interrupt handler? > This will have a negative impact on performance, especially on a kernel > with security mitigations enabled. See also > https://protect2.fireeye.com/v1/url?k=fe14d1e9-a18fe89c-fe155aa6- > 0cc47a31ce4e-8200591154f8c42c&q=1&e=7cf22799-390c-4209-8a19- > 6ad1fa5fa811&u=https%3A%2F%2Flwn.net%2FArticles%2F774743%2F. Interesting. I'll refer to this. Thanks. > > Thanks, > > Bart.
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 05495c34a2b7..f85a9b335e0b 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -6428,6 +6428,16 @@ static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba) static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status) { irqreturn_t retval = IRQ_NONE; + int res = 0; + unsigned long flags; + + retval = ufshcd_vops_intr(hba, &res); + if (res) { + spin_lock_irqsave(hba->host->host_lock, flags); + hba->force_reset = true; + ufshcd_schedule_eh_work(hba); + spin_unlock_irqrestore(hba->host->host_lock, flags); + } if (intr_status & UFSHCD_UIC_MASK) retval |= ufshcd_uic_cmd_compl(hba, intr_status); diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 971cfabc4a1e..1ed0a911f864 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -356,6 +356,7 @@ struct ufs_hba_variant_ops { const union ufs_crypto_cfg_entry *cfg, int slot); void (*event_notify)(struct ufs_hba *hba, enum ufs_event_type evt, void *data); + irqreturn_t (*intr)(struct ufs_hba *hba, int *res); }; /* clock gating state */ @@ -1296,6 +1297,13 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba, hba->vops->config_scaling_param(hba, profile, data); } +static inline irqreturn_t ufshcd_vops_intr(struct ufs_hba *hba, int *res) +{ + if (hba->vops && hba->vops->intr) + return hba->vops->intr(hba, res); + return IRQ_NONE; +} + extern struct ufs_pm_lvl_states ufs_pm_lvl_states[]; /*
This patch is to activate some interrupt sources that aren't defined in UFSHCI specifications. Those purpose could be error handling, workaround or whatever. Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> --- drivers/scsi/ufs/ufshcd.c | 10 ++++++++++ drivers/scsi/ufs/ufshcd.h | 8 ++++++++ 2 files changed, 18 insertions(+)