On Fri, Nov 01, 2024 at 09:31:27AM -0700, Bart Van Assche wrote: > On 11/1/24 12:53 AM, Manivannan Sadhasivam wrote: > > 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@xxxxxxxxxx> > > > Closes: https://lore.kernel.org/linux-scsi/0c0bc528-fdc2-4106-bc99-f23ae377f6f5@xxxxxxxxxx/ > > > Fixes: 6bf999e0eb41 ("scsi: ufs: core: Add UFS RTC support") > > > Cc: Bean Huo <beanhuo@xxxxxxxxxx> > > > Cc: stable@xxxxxxxxxxxxxxx > > > Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx> > > > > 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? > > Hi Manivannan, > > In the context of the Linux kernel, in general, one-line helper > functions are considered questionable. In this case I prefer to keep the > helper functions since these encapsulate an implementation detail, > namely that the WLUN sdev_gendev member is used to control runtime power > management of the UFS host controller. > IMO this encapsulation is causing confusion since we have a separate PM handling for the UFS controller itself. > Checking whether or not the hba->ufs_device_wlun pointer is NULL from > the ufshcd_rtc_work() function would be racy since that pointer is modified > from another thread. So I prefer the patch at the start of this > thread instead of adding a hba->ufs_device_wlun pointer check. > Absolutely! I just pointed out it as a bad example which one could think of due to these helpers. - Mani -- மணிவண்ணன் சதாசிவம்
