| Message ID | 20180807174739.23379-1-vivek.gautam@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/1] scsi/ufshcd: Fix NULL pointer dereference for in ufshcd_init | expand |
On Tue, Aug 7, 2018 at 10:48 AM Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > > Error paths in ufshcd_init() ufshcd_hba_exit() killed clk_scaling > workqueue when the workqueue is actually created quite late in > ufshcd_init(). > So, we end up getting NULL pointer dereference in such error paths. > Fix this by moving clk_scaling initialization and kill codes to > two separate methods, and call them at required places. > > Fixes: 401f1e4490ee ("scsi: ufs: don't suspend clock scaling during clock > gating") > > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > Cc: Subhash Jadavani <subhashj@codeaurora.org> > Cc: Matthias Kaehlcke <mka@chromium.org> > Cc: Evan Green <evgreen@chromium.org> > Cc: Martin K. Petersen <martin.petersen@oracle.com> > --- > > Bjorn, Subhash, > I am not certain of some of these devfreq, and clk_scaling bits > that are moved as part of this patch. Please help in reviewing the > change in the light of these features, and related sequence should > be followed. > Thanks. > You're right, there is a lot of logic moving around here. I think this looks okay to me. Reviewed-by: Evan Green <evgreen@chromium.org>
On Wed, Aug 29, 2018 at 1:13 AM Evan Green <evgreen@chromium.org> wrote: > > On Tue, Aug 7, 2018 at 10:48 AM Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: > > > > Error paths in ufshcd_init() ufshcd_hba_exit() killed clk_scaling > > workqueue when the workqueue is actually created quite late in > > ufshcd_init(). > > So, we end up getting NULL pointer dereference in such error paths. > > Fix this by moving clk_scaling initialization and kill codes to > > two separate methods, and call them at required places. > > > > Fixes: 401f1e4490ee ("scsi: ufs: don't suspend clock scaling during clock > > gating") > > > > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > > Cc: Subhash Jadavani <subhashj@codeaurora.org> > > Cc: Matthias Kaehlcke <mka@chromium.org> > > Cc: Evan Green <evgreen@chromium.org> > > Cc: Martin K. Petersen <martin.petersen@oracle.com> > > --- > > > > Bjorn, Subhash, > > I am not certain of some of these devfreq, and clk_scaling bits > > that are moved as part of this patch. Please help in reviewing the > > change in the light of these features, and related sequence should > > be followed. > > Thanks. > > > > You're right, there is a lot of logic moving around here. I think this > looks okay to me. > > Reviewed-by: Evan Green <evgreen@chromium.org> Thanks Evan. Best regards Vivek
Hi Martin, On Wed, Aug 29, 2018 at 1:50 PM Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > > On Wed, Aug 29, 2018 at 1:13 AM Evan Green <evgreen@chromium.org> wrote: > > > > On Tue, Aug 7, 2018 at 10:48 AM Vivek Gautam > > <vivek.gautam@codeaurora.org> wrote: > > > > > > Error paths in ufshcd_init() ufshcd_hba_exit() killed clk_scaling > > > workqueue when the workqueue is actually created quite late in > > > ufshcd_init(). > > > So, we end up getting NULL pointer dereference in such error paths. > > > Fix this by moving clk_scaling initialization and kill codes to > > > two separate methods, and call them at required places. > > > > > > Fixes: 401f1e4490ee ("scsi: ufs: don't suspend clock scaling during clock > > > gating") > > > > > > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > > > Cc: Subhash Jadavani <subhashj@codeaurora.org> > > > Cc: Matthias Kaehlcke <mka@chromium.org> > > > Cc: Evan Green <evgreen@chromium.org> > > > Cc: Martin K. Petersen <martin.petersen@oracle.com> > > > --- > > > > > > Bjorn, Subhash, > > > I am not certain of some of these devfreq, and clk_scaling bits > > > that are moved as part of this patch. Please help in reviewing the > > > change in the light of these features, and related sequence should > > > be followed. > > > Thanks. > > > > > > > You're right, there is a lot of logic moving around here. I think this > > looks okay to me. > > > > Reviewed-by: Evan Green <evgreen@chromium.org> > > Thanks Evan. > > Best regards > Vivek Gentle ping. Will you please consider picking this patch for the merge? Thanks Vivek
Vivek, >> > > Bjorn, Subhash, >> > > I am not certain of some of these devfreq, and clk_scaling bits >> > > that are moved as part of this patch. Please help in reviewing the >> > > change in the light of these features, and related sequence should >> > > be followed. >> > > Thanks. >> > > >> > >> > You're right, there is a lot of logic moving around here. I think this >> > looks okay to me. >> > >> > Reviewed-by: Evan Green <evgreen@chromium.org> >> >> Thanks Evan. >> >> Best regards >> Vivek > > Gentle ping. Will you please consider picking this patch for the > merge? I was waiting for some feedback from Bjorn or Subhash.
On Wed, Sep 26, 2018 at 6:36 AM Martin K. Petersen <martin.petersen@oracle.com> wrote: > > > Vivek, > > >> > > Bjorn, Subhash, > >> > > I am not certain of some of these devfreq, and clk_scaling bits > >> > > that are moved as part of this patch. Please help in reviewing the > >> > > change in the light of these features, and related sequence should > >> > > be followed. > >> > > Thanks. > >> > > > >> > > >> > You're right, there is a lot of logic moving around here. I think this > >> > looks okay to me. > >> > > >> > Reviewed-by: Evan Green <evgreen@chromium.org> > >> > >> Thanks Evan. > >> > >> Best regards > >> Vivek > > > > Gentle ping. Will you please consider picking this patch for the > > merge? > > I was waiting for some feedback from Bjorn or Subhash. Sure. Bjorn, Subhash, gentle ping. How does this change look to you? Would appreciate if you could please review this. Thanks. Best regards Vivek > > -- > Martin K. Petersen Oracle Linux Engineering
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 397081d320b1..7273aec95a6c 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -1749,6 +1749,34 @@ static ssize_t ufshcd_clkgate_enable_store(struct device *dev, return count; } +static void ufshcd_init_clk_scaling(struct ufs_hba *hba) +{ + char wq_name[sizeof("ufs_clkscaling_00")]; + + if (!ufshcd_is_clkscaling_supported(hba)) + return; + + INIT_WORK(&hba->clk_scaling.suspend_work, + ufshcd_clk_scaling_suspend_work); + INIT_WORK(&hba->clk_scaling.resume_work, + ufshcd_clk_scaling_resume_work); + + snprintf(wq_name, sizeof(wq_name), "ufs_clkscaling_%d", + hba->host->host_no); + hba->clk_scaling.workq = create_singlethread_workqueue(wq_name); + + ufshcd_clkscaling_init_sysfs(hba); +} + +static void ufshcd_exit_clk_scaling(struct ufs_hba *hba) +{ + if (!ufshcd_is_clkscaling_supported(hba)) + return; + + destroy_workqueue(hba->clk_scaling.workq); + ufshcd_devfreq_remove(hba); +} + static void ufshcd_init_clk_gating(struct ufs_hba *hba) { char wq_name[sizeof("ufs_clk_gating_00")]; @@ -6652,6 +6680,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba) */ if (ret && !ufshcd_eh_in_progress(hba) && !hba->pm_op_in_progress) { pm_runtime_put_sync(hba->dev); + ufshcd_exit_clk_scaling(hba); ufshcd_hba_exit(hba); } @@ -7187,12 +7216,9 @@ static void ufshcd_hba_exit(struct ufs_hba *hba) ufshcd_variant_hba_exit(hba); ufshcd_setup_vreg(hba, false); ufshcd_suspend_clkscaling(hba); - if (ufshcd_is_clkscaling_supported(hba)) { + if (ufshcd_is_clkscaling_supported(hba)) if (hba->devfreq) ufshcd_suspend_clkscaling(hba); - destroy_workqueue(hba->clk_scaling.workq); - ufshcd_devfreq_remove(hba); - } ufshcd_setup_clocks(hba, false); ufshcd_setup_hba_vreg(hba, false); hba->is_powered = false; @@ -7867,6 +7893,7 @@ void ufshcd_remove(struct ufs_hba *hba) ufshcd_disable_intr(hba, hba->intr_mask); ufshcd_hba_stop(hba, true); + ufshcd_exit_clk_scaling(hba); ufshcd_exit_clk_gating(hba); if (ufshcd_is_clkscaling_supported(hba)) device_remove_file(hba->dev, &hba->clk_scaling.enable_attr); @@ -8031,6 +8058,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) ufshcd_init_clk_gating(hba); + ufshcd_init_clk_scaling(hba); + /* * In order to avoid any spurious interrupt immediately after * registering UFS controller interrupt handler, clear any pending UFS @@ -8069,21 +8098,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) goto out_remove_scsi_host; } - if (ufshcd_is_clkscaling_supported(hba)) { - char wq_name[sizeof("ufs_clkscaling_00")]; - - INIT_WORK(&hba->clk_scaling.suspend_work, - ufshcd_clk_scaling_suspend_work); - INIT_WORK(&hba->clk_scaling.resume_work, - ufshcd_clk_scaling_resume_work); - - snprintf(wq_name, sizeof(wq_name), "ufs_clkscaling_%d", - host->host_no); - hba->clk_scaling.workq = create_singlethread_workqueue(wq_name); - - ufshcd_clkscaling_init_sysfs(hba); - } - /* * Set the default power management level for runtime and system PM. * Default power saving mode is to keep UFS link in Hibern8 state @@ -8121,6 +8135,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) out_remove_scsi_host: scsi_remove_host(hba->host); exit_gating: + ufshcd_exit_clk_scaling(hba); ufshcd_exit_clk_gating(hba); out_disable: hba->is_irq_enabled = false;
Error paths in ufshcd_init() ufshcd_hba_exit() killed clk_scaling workqueue when the workqueue is actually created quite late in ufshcd_init(). So, we end up getting NULL pointer dereference in such error paths. Fix this by moving clk_scaling initialization and kill codes to two separate methods, and call them at required places. Fixes: 401f1e4490ee ("scsi: ufs: don't suspend clock scaling during clock gating") Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> Cc: Bjorn Andersson <bjorn.andersson@linaro.org> Cc: Subhash Jadavani <subhashj@codeaurora.org> Cc: Matthias Kaehlcke <mka@chromium.org> Cc: Evan Green <evgreen@chromium.org> Cc: Martin K. Petersen <martin.petersen@oracle.com> --- Bjorn, Subhash, I am not certain of some of these devfreq, and clk_scaling bits that are moved as part of this patch. Please help in reviewing the change in the light of these features, and related sequence should be followed. Thanks. drivers/scsi/ufs/ufshcd.c | 53 ++++++++++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 19 deletions(-)