Message-ID: <95007a30-cf83-992c-79bd-80e332a7b6cf@linaro.org>
Date:   Thu, 17 Feb 2022 08:47:37 -0600
From:   Alex Elder <elder@...aro.org>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc:     mhi@...ts.linux.dev, quic_hemantk@...cinc.com,
        quic_bbhatt@...cinc.com, quic_jhugo@...cinc.com,
        vinod.koul@...aro.org, bjorn.andersson@...aro.org,
        dmitry.baryshkov@...aro.org, quic_vbadigan@...cinc.com,
        quic_cang@...cinc.com, quic_skananth@...cinc.com,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 08/25] bus: mhi: ep: Add support for registering MHI
 endpoint controllers

On 2/17/22 3:53 AM, Manivannan Sadhasivam wrote:
> On Tue, Feb 15, 2022 at 02:02:41PM -0600, Alex Elder wrote:
> 
> [...]
> 
>>> +#define MHI_REG_OFFSET				0x100
>>> +#define BHI_REG_OFFSET				0x200

. . .

> [...]
> 
>>> +/* Generic context */
>>> +struct mhi_generic_ctx {
>>> +	__u32 reserved0;
>>> +	__u32 reserved1;
>>> +	__u32 reserved2;
>>> +
>>> +	__u64 rbase __packed __aligned(4);
>>> +	__u64 rlen __packed __aligned(4);
>>> +	__u64 rp __packed __aligned(4);
>>> +	__u64 wp __packed __aligned(4);
>>> +};
>>
>> I'm pretty sure this constitutes an external interface, so
>> every field should have its endianness annotated.
>>
>> Mentioned elsewhere, I think you can define the structure
>> with those attributes rather than the multiple fields.
>>
> 
> As I said before, this was suggested by Arnd during MHI host review. He
> suggested adding the alignment and packed to only members that require
> them.
> 
> But I think I should change it now...

Despite suggesting this more than once, I'm not 100% sure it's
even a correct suggestion.  I trust Arnd's judgement, and I
can see the value of being explicit about *which* fields have
the alignment requirement.  So I'll leave it up to you to
decide...  If you make my suggested change, be sure to test
it.  But I'm fine if you leave these as-is.

>>> +enum mhi_ep_ring_type {
>>> +	RING_TYPE_CMD = 0,
>>> +	RING_TYPE_ER,
>>> +	RING_TYPE_CH,
>>> +};
>>> +
>>> +struct mhi_ep_ring_element {
>>> +	u64 ptr;
>>> +	u32 dword[2];
>>> +};
>>
>> Are these host resident rings?  Even if not, this is an external
>> interface, so this should be defined with explicit endianness.
>> The cpu_to_le64() call will be a no-op so there is no cost
>> to correcting this.
>>
> 
> Ah, this should be reusing the "struct mhi_tre" defined in host. Will do.
> 
>>> +
>>> +/* Ring element */
>>> +union mhi_ep_ring_ctx {
>>> +	struct mhi_cmd_ctxt cmd;
>>> +	struct mhi_event_ctxt ev;
>>> +	struct mhi_chan_ctxt ch;
>>> +	struct mhi_generic_ctx generic;
>>> +};
>>> +
>>> +struct mhi_ep_ring {
>>> +	struct mhi_ep_cntrl *mhi_cntrl;
>>> +	int (*ring_cb)(struct mhi_ep_ring *ring, struct mhi_ep_ring_element *el);
>>> +	union mhi_ep_ring_ctx *ring_ctx;
>>> +	struct mhi_ep_ring_element *ring_cache;
>>> +	enum mhi_ep_ring_type type;
>>> +	size_t rd_offset;
>>> +	size_t wr_offset;
>>> +	size_t ring_size;
>>> +	u32 db_offset_h;
>>> +	u32 db_offset_l;
>>> +	u32 ch_id;
>>> +};
>>
>> Not sure about the db_offset fields, etc. here, but it's possible
>> they need endianness annotations.  I'm going to stop making this
>> comment; please make sure anything that's exposed to the host
>> specifies that it's little endian.  (The host and endpoint should
>> have a common definition of these shared structures anyway; maybe
>> I'm misreading this or assuming something incorrectly.)
>>
> 
> db_offset_* just holds the register offsets so they don't require
> endianness annotation. All MMIO read/write are using readl/writel APIs
> and they handle the endianness conversion implicitly.
> 
> Rest of the host memory accesses are annotated properly.

OK, good.

> 
>>> +
> 
> [...]
> 
>>> +	/*
>>> +	 * Allocate max_channels supported by the MHI endpoint and populate
>>> +	 * only the defined channels
>>> +	 */
>>> +	mhi_cntrl->mhi_chan = kcalloc(mhi_cntrl->max_chan, sizeof(*mhi_cntrl->mhi_chan),
>>> +				      GFP_KERNEL);
>>> +	if (!mhi_cntrl->mhi_chan)
>>> +		return -ENOMEM;
>>> +
>>> +	for (i = 0; i < config->num_channels; i++) {
>>> +		struct mhi_ep_chan *mhi_chan;
>>
>> This entire block could be encapsulated in mhi_channel_add()
>> or something,
> 
> Wrapping up in a function is useful if the same code is used in
> different places. But I don't think it adds any value here.
> 
>>
>>> +		ch_cfg = &config->ch_cfg[i];
>>
>> Move the above assignment down a few lines, to just before
>> where it's used.
>>
> 
> No, ch_cfg is used just below this.

Yes you're right, I missed that.

>>> +
>>> +		chan = ch_cfg->num;
>>> +		if (chan >= mhi_cntrl->max_chan) {
>>> +			dev_err(dev, "Channel %d not available\n", chan);
>>
>> Maybe report the maximum channel so it's obvious why it's
>> not available.
>>
>>> +			goto error_chan_cfg;
>>> +		}
>>> +
>>> +		/* Bi-directional and direction less channels are not supported */
>>> +		if (ch_cfg->dir == DMA_BIDIRECTIONAL || ch_cfg->dir == DMA_NONE) {
>>> +			dev_err(dev, "Invalid channel configuration\n");
>>
>> Maybe be more specific in your message about what's wrong here.
>>
>>> +			goto error_chan_cfg;
>>> +		}
>>> +
>>> +		mhi_chan = &mhi_cntrl->mhi_chan[chan];
>>> +		mhi_chan->name = ch_cfg->name;
>>> +		mhi_chan->chan = chan;
>>> +		mhi_chan->dir = ch_cfg->dir;
>>> +		mutex_init(&mhi_chan->lock);
>>> +	}
>>> +
>>> +	return 0;
>>> +
>>> +error_chan_cfg:
>>> +	kfree(mhi_cntrl->mhi_chan);
>>
>> I'm not sure what the caller does, but maybe null this
>> after it's freed, or don't assign mhi_cntrll->mhi_chan
>> until the initialization is successful.
>>
> 
> This is not required here as there will be no access to the pointer
> after failing.

OK.

>>> +	return ret;
>>> +}
>>> +
>>> +/*
>>> + * Allocate channel and command rings here. Event rings will be allocated
>>> + * in mhi_ep_power_up() as the config comes from the host.
>>> + */
>>> +int mhi_ep_register_controller(struct mhi_ep_cntrl *mhi_cntrl,
>>> +				const struct mhi_ep_cntrl_config *config)
>>> +{
>>> +	struct mhi_ep_device *mhi_dev;
>>> +	int ret;
>>> +
>>> +	if (!mhi_cntrl || !mhi_cntrl->cntrl_dev)
>>> +		return -EINVAL;
>>> +
>>> +	ret = parse_ch_cfg(mhi_cntrl, config);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	mhi_cntrl->mhi_cmd = kcalloc(NR_OF_CMD_RINGS, sizeof(*mhi_cntrl->mhi_cmd), GFP_KERNEL);
>>
>> I said before I thought it was silly to even define NR_OF_CMD_RINGS.
>> Does the MHI specification actually allow more than one command
>> ring for a given MHI controller?  Ever?
>>
> 
> MHI spec doesn't limit the number of command rings. Eventhough I don't
> envision adding more command rings in the future, I'm going to keep this
> macro for now as the MHI host does the same.

OK.

> [...]
> 
>>> diff --git a/include/linux/mhi_ep.h b/include/linux/mhi_ep.h
>>> new file mode 100644
>>> index 000000000000..20238e9df1b3
>>> --- /dev/null
>>> +++ b/include/linux/mhi_ep.h
> 
> [...]
> 
>>> +struct mhi_ep_device {
>>> +	struct device dev;
>>> +	struct mhi_ep_cntrl *mhi_cntrl;
>>> +	const struct mhi_device_id *id;
>>> +	const char *name;
>>> +	struct mhi_ep_chan *ul_chan;
>>> +	struct mhi_ep_chan *dl_chan;
>>> +	enum mhi_device_type dev_type;
>>
>> There are two device types, controller and transfer.  Unless
>> there is ever going to be anything more than that, I think
>> the distinction is better represented as a Boolean, such as:
>>
>> 	bool controller;
> 
> Again, this is how it is done in MHI host also. Since I'm going to
> maintain both stacks, it makes it easier for me if similarities are
> maintained. But I'll keep this suggestion and the one above for later.

Sounds good.  Thanks.

					-Alex

> Thanks,
> Mani

Powered by blists - more mailing lists