Message ID | 20240905220214.738506-3-bvanassche@acm.org |
---|---|
State | New |
Headers | show |
Series | Simplify the UFS driver initialization code | expand |
On 9/5/2024 3:01 PM, Bart Van Assche wrote: > Prepare for introducing a second caller by moving the code for > activating the link between UFS controller and device into a new > function. No functionality has been changed. > > Reviewed-by: Avri Altman <avri.altman@wdc.com> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Reviewed-by: Bean Huo <beanhuo@micron.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/ufs/core/ufshcd.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index ecf6da2efed1..4cfa8dd5500a 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -8709,10 +8709,9 @@ static void ufshcd_config_mcq(struct ufs_hba *hba) > hba->nutrs); > } > > -static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params) > +static int ufshcd_activate_link(struct ufs_hba *hba) > { > int ret; > - struct Scsi_Host *host = hba->host; > > hba->ufshcd_state = UFSHCD_STATE_RESET; > > @@ -8729,6 +8728,18 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params) > /* UniPro link is active now */ > ufshcd_set_link_active(hba); > > + return 0; > +} > + > +static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params) > +{ > + struct Scsi_Host *host = hba->host; > + int ret; > + > + ret = ufshcd_activate_link(hba); > + if (ret) > + return ret; Hi Bart, There may be a problem in this patch. In the original code, if UFSHCD_QUIRK_SKIP_PH_CONFIGURATION is set, ufshcd_device_init() would exit early. However, in this patch, if UFSHCD_QUIRK_SKIP_PH_CONFIGURATION is set, the new function ufshcd_activate_link() would return 0 which would cause the ufshcd_device_init() to continue further down. As a result, the ufshcd_device_init() would fail to handle the UFSHCD_QUIRK_SKIP_PH_CONFIGURATION flag as its original intention, right? Thanks, Bao > + > /* Reconfigure MCQ upon reset */ > if (hba->mcq_enabled && !init_dev_params) { > ufshcd_config_mcq(hba); >
On 9/6/24 4:59 PM, Bao D. Nguyen wrote: > In the original code, if UFSHCD_QUIRK_SKIP_PH_CONFIGURATION is set, > ufshcd_device_init() would exit early. However, in this patch, if > UFSHCD_QUIRK_SKIP_PH_CONFIGURATION is set, the new function > ufshcd_activate_link() would return 0 which would cause the > ufshcd_device_init() to continue further down. As a result, the > ufshcd_device_init() would fail to handle the > UFSHCD_QUIRK_SKIP_PH_CONFIGURATION flag as its original intention, right? Hi Bao, I was assuming that UFSHCD_STATE_RESET != 0 but apparently it is zero. I will fix this patch before I repost this patch series. Thanks, Bart.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index ecf6da2efed1..4cfa8dd5500a 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -8709,10 +8709,9 @@ static void ufshcd_config_mcq(struct ufs_hba *hba) hba->nutrs); } -static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params) +static int ufshcd_activate_link(struct ufs_hba *hba) { int ret; - struct Scsi_Host *host = hba->host; hba->ufshcd_state = UFSHCD_STATE_RESET; @@ -8729,6 +8728,18 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params) /* UniPro link is active now */ ufshcd_set_link_active(hba); + return 0; +} + +static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params) +{ + struct Scsi_Host *host = hba->host; + int ret; + + ret = ufshcd_activate_link(hba); + if (ret) + return ret; + /* Reconfigure MCQ upon reset */ if (hba->mcq_enabled && !init_dev_params) { ufshcd_config_mcq(hba);