Re: [PATCH] scsi: ufs: Start the RTC update work later

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Thu, Oct 31, 2024 at 02:26:24PM -0700, Bart Van Assche wrote:
> The RTC update work involves runtime resuming the UFS controller. Hence,
> only start the RTC update work after runtime power management in the UFS
> driver has been fully initialized. This patch fixes the following kernel
> crash:
> 
> Internal error: Oops: 0000000096000006 [#1] PREEMPT SMP
> Workqueue: events ufshcd_rtc_work
> Call trace:
>  _raw_spin_lock_irqsave+0x34/0x8c (P)
>  pm_runtime_get_if_active+0x24/0x9c (L)
>  pm_runtime_get_if_active+0x24/0x9c
>  ufshcd_rtc_work+0x138/0x1b4
>  process_one_work+0x148/0x288
>  worker_thread+0x2cc/0x3d4
>  kthread+0x110/0x114
>  ret_from_fork+0x10/0x20
> 
> Reported-by: Neil Armstrong <neil.armstrong@linaro.org>
> Closes: https://lore.kernel.org/linux-scsi/0c0bc528-fdc2-4106-bc99-f23ae377f6f5@linaro.org/
> Fixes: 6bf999e0eb41 ("scsi: ufs: core: Add UFS RTC support")
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Bart, Thanks for the fix! While looking into this patch, I also found the
weirdness of the ufshcd_rpm_*() helpers in ufshcd-priv.h. Their naming doesn't
seem to indicate whether those helpers are for WLUN or for HBA. Also, I don't
see the benefit of these helpers since they just wrap generic pm_runtime*
calls. Then there are other open coding instances in the ufshcd.c. Like

pm_runtime_suspended(&hba->ufs_device_wlun->sdev_gendev)
pm_runtime_set_active(&hba->ufs_device_wlun->sdev_gendev)

Moreover, we do check for the presence of hba->ufs_device_wlun before calling
ufshcd_rpm_get_sync() in ufshcd_remove(). This could be one other way to fix
this null ptr dereference even though I wouldn't recommend doing so as calling
rtc_work early is pointless.

So I think we should remove these helpers to avoid having these discrepancies.
WDYT?

- Mani

-- 
மணிவண்ணன் சதாசிவம்