linux-kernel - Re: [PATCH v4 04/11] bus: mhi: host: Add support for Bandwidth scale

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <76txeoaa7k3nquvegvmivjazlzdtsnsxa3jtfrlfzbndffc7dx@c2nfzty73scj>
Date: Wed, 23 Jul 2025 21:55:31 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
Cc: Jeff Johnson <jeff.johnson@....qualcomm.com>, 
	Jeff Johnson <jjohnson@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, Jingoo Han <jingoohan1@...il.com>, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Rob Herring <robh@...nel.org>, 
	Bartosz Golaszewski <brgl@...ev.pl>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	mhi@...ts.linux.dev, linux-wireless@...r.kernel.org, ath11k@...ts.infradead.org, 
	qiang.yu@....qualcomm.com, quic_vbadigan@...cinc.com, quic_vpernami@...cinc.com, 
	quic_mrana@...cinc.com
Subject: Re: [PATCH v4 04/11] bus: mhi: host: Add support for Bandwidth scale

On Fri, Jul 11, 2025 at 12:25:30PM GMT, Krishna Chaitanya Chundru wrote:

[...]

> > > > > +static int mhi_init_bw_scale(struct mhi_controller *mhi_cntrl,
> > > > > +			     int bw_scale_db)
> > > > > +{
> > > > > +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> > > > > +	u32 bw_cfg_offset, val;
> > > > > +	int ret, er_index;
> > > > > +
> > > > > +	ret = mhi_find_capability(mhi_cntrl, MHI_BW_SCALE_CAP_ID, &bw_cfg_offset);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	er_index = mhi_get_er_index(mhi_cntrl, MHI_ER_BW_SCALE);
> > > > > +	if (er_index < 0)
> > > > > +		return er_index;
> > > > > +
> > > > > +	bw_cfg_offset += MHI_BW_SCALE_CFG_OFFSET;
> > > > > +
> > > > > +	/* Advertise host support */
> > > > > +	val = (__force u32)cpu_to_le32(FIELD_PREP(MHI_BW_SCALE_DB_CHAN_ID, bw_scale_db) |
> > > > > +				       FIELD_PREP(MHI_BW_SCALE_ER_INDEX, er_index) |
> > > > > +				       MHI_BW_SCALE_ENABLED);
> > > > > +
> > > > 
> > > > It is wrong to store the value of cpu_to_le32() in a non-le32 variable.
> > > > mhi_write_reg() accepts the 'val' in native endian. writel used in the
> > > > controller drivers should take care of converting to LE before writing to the
> > > > device.
> > > > 
> > > ok then I will revert to u32.
> > > 
> > > I think we need a patch in the controller drivers seperately to handle
> > > this.
> > 
> > Why?
> > 
> what I understood from your previous comment is from here we need to
> send u32 only and the controller drivers should take care of
> converting u32 to le32.
> As of today controller drivers are not considering this and writing
> u32 only.
> So we need a seperate patch in the controller driver to convert it to
> le32.

No. All controller drivers are using writel() to write the register. Since
writel() converts the value to little endian from native endian, you do not need
to do anything.

> > > > > +	mhi_write_reg(mhi_cntrl, mhi_cntrl->regs, bw_cfg_offset, val);
> > > > > +
> > > > > +	dev_dbg(dev, "Bandwidth scaling setup complete with event ring: %d\n",
> > > > > +		er_index);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +

[...]

> > > > > +	link_info.target_link_speed = MHI_TRE_GET_EV_LINKSPEED(dev_rp);
> > > > > +	link_info.target_link_width = MHI_TRE_GET_EV_LINKWIDTH(dev_rp);
> > > > > +	link_info.sequence_num = MHI_TRE_GET_EV_BW_REQ_SEQ(dev_rp);
> > > > > +
> > > > > +	dev_dbg(dev, "Received BW_REQ with seq:%d link speed:0x%x width:0x%x\n",
> > > > > +		link_info.sequence_num,
> > > > > +		link_info.target_link_speed,
> > > > > +		link_info.target_link_width);
> > > > > +
> > > > > +	/* Bring host and device out of suspended states */
> > > > > +	ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
> > > > 
> > > > Looks like mhi_device_get_sync() is going runtime_get()/runtime_put() inside
> > > > mhi_trigger_resume(). I'm wondering why that is necessary.
> > > > 
> > > Before mhi_trigger_resume we are doing wake_get, which will make sure
> > > device will not transition to the low power modes while servicing this
> > > event. And also make sure mhi state is in M0 only.
> > > 
> > > As we are in workqueue this can be scheduled at some later time and by
> > > that time mhi can go to m1 or m2 state.
> > > 
> > 
> > My comment was more about the behavior of mhi_trigger_resume(). Why does it call
> > get() and put()? Sorry if I was not clear.
> > Get() needed for bringing out of suspend, put() is for balancing the
> get() I belive the intention here is to trigger resume only and balance
> pm framework.
> 
> That said mhi_device_get_sync() has a bug we are doing wake_get() before
> triggering resume and also trigger resume will not guarantee that device
> had resumed, it is not safe to do wake_get untill device is out of
> suspend, we might need to introduce runtime_get_sync() function and new
> API for this purpose.
> mhi_trigger_resume makes sense in the place like this[1], where ever
> there are register writes after trigger resume it may need  to be
> replaced with runtime_get_sync().
> 
> This function needs to do mhi_cntrl->runtime_get_sync(mhi_cntrl); first
> to make sure controller is not in suspend and then mhi_device_get_sync()
> to make sure device is staying in M0. and in the end we balance these
> with mhi_cntrl->runtime_put(mhi_cntrl) & mhi_device_put().
> 

Sounds good to me.

- Mani

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