linux-kernel - Re: [PATCH 07/20] bus: mhi: ep: Add support for creating and destroying MHI EP devices

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 5 Jan 2022 18:28:08 -0600
From:   Alex Elder <elder@...e.org>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        mhi@...ts.linux.dev
Cc:     hemantk@...eaurora.org, bbhatt@...eaurora.org,
        quic_jhugo@...cinc.com, vinod.koul@...aro.org,
        bjorn.andersson@...aro.org, dmitry.baryshkov@...aro.org,
        skananth@...eaurora.org, vpernami@...eaurora.org,
        vbadigan@...eaurora.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 07/20] bus: mhi: ep: Add support for creating and
 destroying MHI EP devices

On 12/2/21 5:35 AM, Manivannan Sadhasivam wrote:
> This commit adds support for creating and destroying MHI endpoint devices.
> The MHI endpoint devices binds to the MHI endpoint channels and are used
> to transfer data between MHI host and endpoint device.
> 
> There is a single MHI EP device for each channel pair. The devices will be
> created when the corresponding channels has been started by the host and
> will be destroyed during MHI EP power down and reset.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> ---
>   drivers/bus/mhi/ep/main.c | 85 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 85 insertions(+)
> 
> diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
> index ce0f99f22058..f0b5f49db95a 100644
> --- a/drivers/bus/mhi/ep/main.c
> +++ b/drivers/bus/mhi/ep/main.c
> @@ -63,6 +63,91 @@ static struct mhi_ep_device *mhi_ep_alloc_device(struct mhi_ep_cntrl *mhi_cntrl)
>   	return mhi_dev;
>   }
>   
> +static int mhi_ep_create_device(struct mhi_ep_cntrl *mhi_cntrl, u32 ch_id)
> +{
> +	struct mhi_ep_device *mhi_dev;
> +	struct mhi_ep_chan *mhi_chan = &mhi_cntrl->mhi_chan[ch_id];
> +	int ret;
> +
> +	mhi_dev = mhi_ep_alloc_device(mhi_cntrl);
> +	if (IS_ERR(mhi_dev))
> +		return PTR_ERR(mhi_dev);
> +
> +	mhi_dev->dev_type = MHI_DEVICE_XFER;

Elsewhere (at least in mhi_ep_process_tre_ring()) in your code
you assume that the even-numbered channel is UL.

I would say, either use that assumption throughout, or do
not use that assumption at all.  (I prefer the latter.)

I don't really like how this assumes that the channels
are defined in adjacent pairs.  It assumes one is
upload and the next one is download, but doesn't
specify the order in which they're defined.  If
you're going to assume they are defined in pairs, you
should be able to assume which one is defined first,
and then simplify this code (and even verify that
they are defined UL before DL, perhaps).

> +	/* Configure primary channel */
> +	if (mhi_chan->dir == DMA_TO_DEVICE) {
> +		mhi_dev->ul_chan = mhi_chan;
> +		mhi_dev->ul_chan_id = mhi_chan->chan;
> +	} else {
> +		mhi_dev->dl_chan = mhi_chan;
> +		mhi_dev->dl_chan_id = mhi_chan->chan;
> +	}
> +
> +	get_device(&mhi_dev->dev);
> +	mhi_chan->mhi_dev = mhi_dev;
> +
> +	/* Configure secondary channel as well */
> +	mhi_chan++;
> +	if (mhi_chan->dir == DMA_TO_DEVICE) {
> +		mhi_dev->ul_chan = mhi_chan;
> +		mhi_dev->ul_chan_id = mhi_chan->chan;
> +	} else {
> +		mhi_dev->dl_chan = mhi_chan;
> +		mhi_dev->dl_chan_id = mhi_chan->chan;
> +	}
> +
> +	get_device(&mhi_dev->dev);
> +	mhi_chan->mhi_dev = mhi_dev;
> +
> +	/* Channel name is same for both UL and DL */

You could verify the two channels indeed have the
same name.

					-Alex

> +	mhi_dev->name = mhi_chan->name;
> +	dev_set_name(&mhi_dev->dev, "%s_%s",
> +		     dev_name(&mhi_cntrl->mhi_dev->dev),
> +		     mhi_dev->name);
> +
> +	ret = device_add(&mhi_dev->dev);
> +	if (ret)
> +		put_device(&mhi_dev->dev);
> +
> +	return ret;
> +}
> +
> +static int mhi_ep_destroy_device(struct device *dev, void *data)
> +{
> +	struct mhi_ep_device *mhi_dev;
> +	struct mhi_ep_cntrl *mhi_cntrl;
> +	struct mhi_ep_chan *ul_chan, *dl_chan;
> +
> +	if (dev->bus != &mhi_ep_bus_type)
> +		return 0;
> +
> +	mhi_dev = to_mhi_ep_device(dev);
> +	mhi_cntrl = mhi_dev->mhi_cntrl;
> +
> +	/* Only destroy devices created for channels */
> +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
> +		return 0;
> +
> +	ul_chan = mhi_dev->ul_chan;
> +	dl_chan = mhi_dev->dl_chan;
> +
> +	if (ul_chan)
> +		put_device(&ul_chan->mhi_dev->dev);
> +
> +	if (dl_chan)
> +		put_device(&dl_chan->mhi_dev->dev);
> +
> +	dev_dbg(&mhi_cntrl->mhi_dev->dev, "Destroying device for chan:%s\n",
> +		 mhi_dev->name);
> +
> +	/* Notify the client and remove the device from MHI bus */
> +	device_del(dev);
> +	put_device(dev);
> +
> +	return 0;
> +}
> +
>   static int parse_ch_cfg(struct mhi_ep_cntrl *mhi_cntrl,
>   			const struct mhi_ep_cntrl_config *config)
>   {
>