| 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
| ||
|
Message-ID: <3bf88d90e4006ba17e2e86c76a926581@codeaurora.org>
Date: Mon, 16 Nov 2020 12:56:16 -0800
From: Bhaumik Bhatt <bbhatt@...eaurora.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: linux-arm-msm@...r.kernel.org, hemantk@...eaurora.org,
jhugo@...eaurora.org, loic.poulain@...aro.org,
kvalo@...eaurora.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/6] bus: mhi: core: Add support to stop or start
channel data transfers
Hi Mani,
On 2020-11-16 04:43, Manivannan Sadhasivam wrote:
> On Wed, Nov 11, 2020 at 11:21:10AM -0800, Bhaumik Bhatt wrote:
>> Some MHI client drivers may want to request a pause or halt of
>> data transfer activity on their channels. Support for this does
>> not exist and must be introduced, wherein the channel context is
>> not reset or cleared but only the STOP channel command is issued.
>> This would need to be paired with an API that allows resuming the
>> data transfer activity on channels by use of the START channel
>> command. This API assumes that the context information is already
>> setup. Enable this using two new APIs, mhi_start_transfer() and
>> mhi_stop_transfer().
>>
>> Signed-off-by: Bhaumik Bhatt <bbhatt@...eaurora.org>
>> ---
>> drivers/bus/mhi/core/main.c | 41
>> +++++++++++++++++++++++++++++++++++++++++
>> include/linux/mhi.h | 19 +++++++++++++++++++
>> 2 files changed, 60 insertions(+)
>>
>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>> index 1226933..1a969f4 100644
>> --- a/drivers/bus/mhi/core/main.c
>> +++ b/drivers/bus/mhi/core/main.c
>> @@ -1560,6 +1560,47 @@ void mhi_unprepare_from_transfer(struct
>> mhi_device *mhi_dev)
>> }
>> EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
>>
>> +static int mhi_update_transfer_state(struct mhi_device *mhi_dev,
>> + enum mhi_ch_state_type to_state)
>> +{
>> + struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>> + struct mhi_chan *mhi_chan;
>> + int dir, ret;
>> +
>> + for (dir = 0; dir < 2; dir++) {
>> + mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
>> +
>> + if (!mhi_chan)
>> + continue;
>> +
>> + /*
>> + * Bail out if one of the channels fail as client will reset
>> + * both upon failure
>> + */
>> + mutex_lock(&mhi_chan->mutex);
>
> Hmm. The documentation about wait_for_completion*() used in
> mhi_update_channel_state()says below,
>
> "As all variants of wait_for_completion() can (obviously) block for a
> long
> time depending on the nature of the activity they are waiting for, so
> in
> most cases you probably don't want to call this with held mutexes."
>
Yes, that is understood. The mhi_chan->mutex is only used to lock any
channel
enable/start/stop/disable type operations, since these have to be in
order, it
is essential that we wait for one of the operations to finish before the
next
one.
Also we avoid a race, for example, at a time when a device crash forces
a driver
"remove" call, while an operation to start/stop a channel is already
going on.
>> + ret = mhi_update_channel_state(mhi_cntrl, mhi_chan, to_state);
>> + if (ret) {
>> + mutex_unlock(&mhi_chan->mutex);
>> + return ret;
>> + }
>> + mutex_unlock(&mhi_chan->mutex);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int mhi_stop_transfer(struct mhi_device *mhi_dev)
>> +{
>> + return mhi_update_transfer_state(mhi_dev, MHI_CH_STATE_TYPE_STOP);
>> +}
>> +EXPORT_SYMBOL_GPL(mhi_stop_transfer);
>> +
>> +int mhi_start_transfer(struct mhi_device *mhi_dev)
>> +{
>> + return mhi_update_transfer_state(mhi_dev, MHI_CH_STATE_TYPE_START);
>> +}
>> +EXPORT_SYMBOL_GPL(mhi_start_transfer);
>> +
>> int mhi_poll(struct mhi_device *mhi_dev, u32 budget)
>> {
>> struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>> index 52b3c60..aee8494 100644
>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -702,6 +702,25 @@ int mhi_prepare_for_transfer(struct mhi_device
>> *mhi_dev);
>> void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev);
>>
>> /**
>> + * mhi_stop_transfer - Pauses ongoing channel activity by issuing the
>> STOP
>> + * channel command to both UL and DL channels.
>> This command
>> + * does not reset the channel context and the
>> client drivers
>> + * can issue mhi_start_transfer to resume
>> activity.
>> + * @mhi_dev: Device associated with the channels
>> + */
>> +int mhi_stop_transfer(struct mhi_device *mhi_dev);
>> +
>> +/**
>> + * mhi_start_transfer - Resumes channel activity by issuing the START
>> channel
>> + * command to both UL and DL channels. This
>> command assumes
>> + * the channel context is already setup and the
>> client
>> + * drivers can issue mhi_stop_transfer to pause
>> activity if
>> + * required.
>> + * @mhi_dev: Device associated with the channels
>> + */
>> +int mhi_start_transfer(struct mhi_device *mhi_dev);
>> +
>> +/**
>
> Align the comment header properly.
>
So I am trying to follow the documentation style for other functions in
the same
file. Is there any particular format you want me to refer to?
I use all spaces for the lines after the first one to align them just
like the
rest of them.
> Thanks,
> Mani
>
>> * mhi_poll - Poll for any available data in DL direction
>> * @mhi_dev: Device associated with the channels
>> * @budget: # of events to process
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project
>>
Thanks,
Bhaumik
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
Powered by blists - more mailing lists