Re: [PATCH v2 2/2] ufs: serialize AHIT register access between sysfs and host driver

["Peter Wang (王信友)" <peter.wang@mediatek.com>]

On Mon, 2026-09-14 at 09:13 +0000, sashiko-bot@kernel.org wrote:
> [Severity: High]
> This isn't a bug introduced by this patch, but can this dereference a
> NULL pointer?
> 
> Sysfs attributes are exposed in ufshcd_init() via
> ufs_sysfs_add_nodes()
> before hba->ufs_device_wlun is allocated.  The initialization is
> deferred
> to the ufshcd_async_scan worker.
> 
> If a user writes to the sysfs node before ufshcd_add_lus() allocates
> hba->ufs_device_wlun, can auto_hibern8_store() call
> ufshcd_auto_hibern8_update() and locklessly dereference a NULL
> pointer here?
> 

This patch focuses on the current issue.
The previous issue can be addressed with a separate patch, 
but not with this one.

> >  		ufshcd_rpm_get_sync(hba);
> 
> [Severity: High]
> This is a pre-existing issue, but is it safe to ignore the return
> value
> of ufshcd_rpm_get_sync() here?
> 
> If the runtime PM resume fails (e.g., due to a power domain failure
> or
> device removal), pm_runtime_get_sync() returns a negative error code.
> 
> Does ignoring this error allow the function to proceed and write to
> REG_AUTO_HIBERNATE_IDLE_TIMER on a potentially suspended device,
> causing a
> synchronous external abort?
> 

Same, pre-existing issue should be addressed with a separate patch.


> >  		ufshcd_hold(hba);
> > -		ufshcd_configure_auto_hibern8(hba);
> > +		/*
> > +		 * Serialize with the host driver's AHIT control.
> > Skip the
> > +		 * register write while the driver has AHIT forced
> > off; the
> > +		 * driver programs hba->ahit back when it re-
> > enables it.
> > +		 * Mutex is taken here (not across the rpm calls
> > above) so it
> > +		 * never blocks a resume that the driver's path
> > may need.
> > +		 */
> > +		mutex_lock(&hba->ahit_mutex);
> > +		if (!hba->ahit_disable_depth)
> > +			ufshcd_configure_auto_hibern8(hba);
> > +		mutex_unlock(&hba->ahit_mutex);
> >  		ufshcd_release(hba);
> >  		ufshcd_rpm_put_sync(hba);
> >  	}
> 
> [ ... ]
> 
> [Severity: High]
> This is a pre-existing issue, but does the UFS core driver handle
> shared
> IRQs correctly?
> 

Same, pre-existing issue should be addressed with a separate patch.


> The driver registers its IRQ with IRQF_SHARED. When the UFS
> controller enters
> runtime suspend, ufshcd_suspend() disables the interrupt line
> entirely:
> 
> drivers/ufs/core/ufshcd.c:ufshcd_suspend() {
>     ...
> 	ufshcd_disable_irq(hba);
>     ...
> }
> 
> Does calling disable_irq() mask interrupts for any other devices
> sharing the
> line? Should this use synchronize_irq() instead?
> 
> Additionally, ufshcd_intr() reads the hardware register
> unconditionally:
> 
> drivers/ufs/core/ufshcd.c:ufshcd_intr() {
>     ...
> 	last_intr_status = intr_status = ufshcd_readl(hba,
> REG_INTERRUPT_STATUS);
>     ...
> }
> 
> Can this access powered-off hardware if the interrupt belongs to
> another
> device on the same shared line? Should this verify the device is
> actively
> powered using pm_runtime_get_if_active() before accessing registers?
>