| 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
| ||
|
Date: Tue, 16 Apr 2024 13:45:18 +0800
From: Qiang Yu <quic_qianyu@...cinc.com>
To: Manivannan Sadhasivam <mani@...nel.org>
CC: <quic_jhugo@...cinc.com>, <mhi@...ts.linux.dev>,
<linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<quic_cang@...cinc.com>, <quic_mrana@...cinc.com>
Subject: Re: [PATCH v3 1/3] bus: mhi: host: Add sysfs entry to force device to
enter EDL
On 4/15/2024 7:56 PM, Manivannan Sadhasivam wrote:
> On Mon, Apr 15, 2024 at 04:49:03PM +0800, Qiang Yu wrote:
>> Add sysfs entry to allow users of MHI bus force device to enter EDL.
>> Considering that the way to enter EDL mode varies from device to device and
>> some devices even do not support EDL. Hence, add a callback edl_trigger in
>> mhi controller as part of the sysfs entry to be invoked and MHI core will
>> only create EDL sysfs entry for mhi controller that provides edl_trigger
>> callback. All of the process a specific device required to enter EDL mode
>> can be placed in this callback.
>>
>> Signed-off-by: Qiang Yu <quic_qianyu@...cinc.com>
>> ---
>> Documentation/ABI/stable/sysfs-bus-mhi | 11 +++++++++++
>> drivers/bus/mhi/host/init.c | 35 ++++++++++++++++++++++++++++++++++
>> include/linux/mhi.h | 2 ++
>> 3 files changed, 48 insertions(+)
>>
>> diff --git a/Documentation/ABI/stable/sysfs-bus-mhi b/Documentation/ABI/stable/sysfs-bus-mhi
>> index 1a47f9e..d0bf9ae 100644
>> --- a/Documentation/ABI/stable/sysfs-bus-mhi
>> +++ b/Documentation/ABI/stable/sysfs-bus-mhi
>> @@ -29,3 +29,14 @@ Description: Initiates a SoC reset on the MHI controller. A SoC reset is
>> This can be useful as a method of recovery if the device is
>> non-responsive, or as a means of loading new firmware as a
>> system administration task.
>> +
>> +What: /sys/bus/mhi/devices/.../force_edl
> s/force_edl/trigger_edl
>
>> +Date: April 2024
>> +KernelVersion: 6.9
>> +Contact: mhi@...ts.linux.dev
>> +Description: Force devices to enter EDL (emergency download) mode. Only MHI
> How can the user trigger EDL mode? Writing to this file? If so, what is the
> value?
User can trigger EDL mode by writing a non-zero value to this file.
>
> 'Emergency Download'
>
>> + controller that supports EDL mode and provides a mechanism for
>> + manually triggering EDL contains this file. Once in EDL mode,
> 'This entry only exists for devices capable of entering the EDL mode using the
> standard EDL triggering mechanism defined in the MHI spec <insert the version>.'
>
>> + the flash programmer image can be downloaded to the device to
>> + enter the flash programmer execution environment. This can be
>> + useful if user wants to update firmware.
> It'd be good to mention the OS tool like QDL that is used to download firmware
> over EDL.
OK, can I add an additionnal line like this
Users: Any OS tools that is used to download firmware over EDL
like QDL.
>
>> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
>> index 44f9349..333ac94 100644
>> --- a/drivers/bus/mhi/host/init.c
>> +++ b/drivers/bus/mhi/host/init.c
>> @@ -127,6 +127,32 @@ static ssize_t soc_reset_store(struct device *dev,
>> }
>> static DEVICE_ATTR_WO(soc_reset);
>>
>> +static ssize_t force_edl_store(struct device *dev,
> s/force_edl/trigger_edl
>
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct mhi_device *mhi_dev = to_mhi_device(dev);
>> + struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>> + unsigned long val;
>> + int ret;
>> +
>> + ret = kstrtoul(buf, 10, &val);
>> + if (ret < 0) {
>> + dev_err(dev, "Could not parse string: %d\n", ret);
> No need to print error.
>
>> + return ret;
>> + }
>> +
>> + if (!val)
>> + return count;
> What does this mean?
If user want to trigger EDL mode,he has to write a non-zero value to this
file. If he writes zero, nothing will happen.
Do we need to limit the valid value to a specific value like 1?
>
>> +
>> + ret = mhi_cntrl->edl_trigger(mhi_cntrl);
>> + if (ret)
>> + return ret;
>> +
>> + return count;
>> +}
>> +static DEVICE_ATTR_WO(force_edl);
>> +
>> static struct attribute *mhi_dev_attrs[] = {
>> &dev_attr_serial_number.attr,
>> &dev_attr_oem_pk_hash.attr,
>> @@ -1018,6 +1044,12 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>> if (ret)
>> goto err_release_dev;
>>
>> + if (mhi_cntrl->edl_trigger) {
>> + ret = sysfs_create_file(&mhi_dev->dev.kobj, &dev_attr_force_edl.attr);
>> + if (ret)
>> + goto err_release_dev;
>> + }
>> +
>> mhi_cntrl->mhi_dev = mhi_dev;
>>
>> mhi_create_debugfs(mhi_cntrl);
>> @@ -1051,6 +1083,9 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
>> mhi_deinit_free_irq(mhi_cntrl);
>> mhi_destroy_debugfs(mhi_cntrl);
>>
>> + if (mhi_cntrl->edl_trigger)
>> + sysfs_remove_file(&mhi_dev->dev.kobj, &dev_attr_force_edl.attr);
>> +
>> destroy_workqueue(mhi_cntrl->hiprio_wq);
>> kfree(mhi_cntrl->mhi_cmd);
>> kfree(mhi_cntrl->mhi_event);
>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>> index cde01e1..8280545 100644
>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -353,6 +353,7 @@ struct mhi_controller_config {
>> * @read_reg: Read a MHI register via the physical link (required)
>> * @write_reg: Write a MHI register via the physical link (required)
>> * @reset: Controller specific reset function (optional)
>> + * @edl_trigger: CB function to enter EDL mode (optional)
> s/enter/trigger
>
> - Mani
>
Powered by blists - more mailing lists