From: bbhatt@codeaurora.org
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: linux-arm-msm@vger.kernel.org, hemantk@codeaurora.org,
jhugo@codeaurora.org, linux-kernel@vger.kernel.org,
linux-arm-msm-owner@vger.kernel.org
Subject: Re: [PATCH v5 10/10] bus: mhi: core: Introduce sysfs entries for MHI
Date: Mon, 27 Jul 2020 17:29:44 -0700 [thread overview]
Message-ID: <ebca0d6ccce616fec6fd0fc08230c19e@codeaurora.org> (raw)
In-Reply-To: <20200724054206.GC17957@Mani-XPS-13-9360>
On 2020-07-23 22:42, Manivannan Sadhasivam wrote:
> On Thu, Jul 23, 2020 at 03:36:42PM -0700, Bhaumik Bhatt wrote:
>> Introduce sysfs entries to enable userspace clients the ability to
>> read
>> the serial number and the OEM PK Hash values obtained from BHI. OEMs
>> need to read these device-specific hardware information values through
>> userspace for factory testing purposes and cannot be exposed via
>> degbufs
>> as it may remain disabled for performance reasons. Also, update the
>> documentation for ABI to include these entries.
>>
>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> ---
>> Documentation/ABI/stable/sysfs-bus-mhi | 25 ++++++++++++++++
>> MAINTAINERS | 1 +
>> drivers/bus/mhi/core/init.c | 53
>> ++++++++++++++++++++++++++++++++++
>> 3 files changed, 79 insertions(+)
>> create mode 100644 Documentation/ABI/stable/sysfs-bus-mhi
>>
>> diff --git a/Documentation/ABI/stable/sysfs-bus-mhi
>> b/Documentation/ABI/stable/sysfs-bus-mhi
>> new file mode 100644
>> index 0000000..a4e4bd2
>> --- /dev/null
>> +++ b/Documentation/ABI/stable/sysfs-bus-mhi
>> @@ -0,0 +1,25 @@
>> +What: /sys/bus/mhi/devices/.../serialnumber
>> +Date: July 2020
>> +KernelVersion: 5.8
>> +Contact: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> +Description:
>> + The file holds the serial number of the client device obtained
>> + using a BHI (Boot Host Interface) register read after at least
>> + one attempt to power up the device has been done. If read
>> + without having the device power on at least once, the file will
>> + read all 0's.
>> +Users: Any userspace application or clients interested in the device
>> + hardware information.
>
> Please align all the fields onto a single starting point. Have a look
> at other
> ABI documentation like, Documentation/ABI/stable/sysfs-bus-vmbus.
>
Alignment was updated. Seems OK to me actually, I am unsure why the
patch shows up as
slightly different on email.
>> +
>> +What: /sys/bus/mhi/devices/.../oem_pk_hash
>> +Date: July 2020
>> +KernelVersion: 5.8
>> +Contact: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> +Description:
>> + The file holds the OEM PK Hash value of the endpoint device
>> + obtained using a BHI (Boot Host Interface) register read after
>> + at least one attempt to power up the device has been done. If
>> + read without having the device power on at least once, the file
>> + will read all 0's.
>> +Users: Any userspace application or clients interested in the device
>> + hardware information.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index e64e5db..5e49316 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11018,6 +11018,7 @@ M: Hemant Kumar <hemantk@codeaurora.org>
>> L: linux-arm-msm@vger.kernel.org
>> S: Maintained
>> T: git git://git.kernel.org/pub/scm/linux/kernel/git/mani/mhi.git
>> +F: Documentation/ABI/stable/sysfs-bus-mhi
>> F: Documentation/mhi/
>> F: drivers/bus/mhi/
>> F: include/linux/mhi.h
>> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
>> index d2c0f6e..a7b0d76 100644
>> --- a/drivers/bus/mhi/core/init.c
>> +++ b/drivers/bus/mhi/core/init.c
>> @@ -76,6 +76,56 @@ const char *to_mhi_pm_state_str(enum mhi_pm_state
>> state)
>> return mhi_pm_state_str[index];
>> }
>>
>> +static ssize_t serial_number_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>
> We haven't followed this before but it is good to align the function
> parameters
> with respect to '('.
>
This one too, I have made sure it is aligned with the '('. Maybe a
re-upload should
clear it up.
>> +{
>> + struct mhi_device *mhi_dev = to_mhi_device(dev);
>> + struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>> +
>> + return snprintf(buf, PAGE_SIZE, "Serial Number: %u\n",
>> + mhi_cntrl->serial_number);
>
> We need to think about what happens if the mhi_cntrl structure is not
> zero
> initialized by the controller driver. All throughout the stack we
> assume that
> the mhi_cntrl struct is zero initialized but things can go awry if it
> was not
> the case!
>
> There was one API in the downstream (mhi_alloc_controller()) for this
> purpose
> but I removed it since we ended up with just a kzalloc(). Does it make
> sense to
> introduce it now?
>
Thanks for pointing out. I realize this could have potential
consequences and have added
the patch to introduce the API as a dependency.
> Thanks,
> Mani
>
>> +}
>> +static DEVICE_ATTR_RO(serial_number);
>> +
>> +static ssize_t oem_pk_hash_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct mhi_device *mhi_dev = to_mhi_device(dev);
>> + struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>> + int i, cnt = 0;
>> +
>> + for (i = 0; i < ARRAY_SIZE(mhi_cntrl->oem_pk_hash); i++)
>> + cnt += snprintf(buf + cnt, PAGE_SIZE - cnt,
>> + "OEMPKHASH[%d]: 0x%x\n", i,
>> + mhi_cntrl->oem_pk_hash[i]);
>> +
>> + return cnt;
>> +}
>> +static DEVICE_ATTR_RO(oem_pk_hash);
>> +
>> +static struct attribute *mhi_sysfs_attrs[] = {
>> + &dev_attr_serial_number.attr,
>> + &dev_attr_oem_pk_hash.attr,
>> + NULL,
>> +};
>> +
>> +static const struct attribute_group mhi_sysfs_group = {
>> + .attrs = mhi_sysfs_attrs,
>> +};
>> +
>> +static int mhi_create_sysfs(struct mhi_controller *mhi_cntrl)
>> +{
>> + return sysfs_create_group(&mhi_cntrl->mhi_dev->dev.kobj,
>> + &mhi_sysfs_group);
>> +}
>> +
>> +static void mhi_destroy_sysfs(struct mhi_controller *mhi_cntrl)
>> +{
>> + sysfs_remove_group(&mhi_cntrl->mhi_dev->dev.kobj, &mhi_sysfs_group);
>> +}
>> +
>> /* MHI protocol requires the transfer ring to be aligned with ring
>> length */
>> static int mhi_alloc_aligned_ring(struct mhi_controller *mhi_cntrl,
>> struct mhi_ring *ring,
>> @@ -917,6 +967,8 @@ int mhi_register_controller(struct mhi_controller
>> *mhi_cntrl,
>> mhi_cntrl->mhi_dev = mhi_dev;
>>
>> mhi_create_debugfs(mhi_cntrl);
>> + if (mhi_create_sysfs(mhi_cntrl))
>> + dev_err(mhi_cntrl->cntrl_dev, "Failed to create sysfs entries\n");
>>
>> return 0;
>>
>> @@ -940,6 +992,7 @@ void mhi_unregister_controller(struct
>> mhi_controller *mhi_cntrl)
>> struct mhi_chan *mhi_chan = mhi_cntrl->mhi_chan;
>> unsigned int i;
>>
>> + mhi_destroy_sysfs(mhi_cntrl);
>> mhi_destroy_debugfs(mhi_cntrl);
>>
>> kfree(mhi_cntrl->mhi_cmd);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project
>>
prev parent reply other threads:[~2020-07-28 0:29 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-23 22:36 [PATCH v5 00/10] Introduce features and debugfs/sysfs entries for MHI Bhaumik Bhatt
2020-07-23 22:36 ` [PATCH v5 01/10] bus: mhi: core: Remove double occurrence for mhi_ctrl_ev_task() declaration Bhaumik Bhatt
2020-07-23 22:36 ` [PATCH v5 02/10] bus: mhi: core: Abort suspends due to outgoing pending packets Bhaumik Bhatt
2020-07-23 22:36 ` [PATCH v5 03/10] bus: mhi: core: Use helper API to trigger a non-blocking host resume Bhaumik Bhatt
2020-07-24 6:09 ` Manivannan Sadhasivam
2020-07-23 22:36 ` [PATCH v5 04/10] bus: mhi: core: Trigger host resume if suspended during mhi_device_get() Bhaumik Bhatt
2020-07-23 22:36 ` [PATCH v5 05/10] bus: mhi: core: Use generic name field for an MHI device Bhaumik Bhatt
2020-07-23 22:36 ` [PATCH v5 06/10] bus: mhi: core: Introduce helper function to check device state Bhaumik Bhatt
2020-07-23 22:36 ` [PATCH v5 07/10] bus: mhi: core: Introduce debugfs entries for MHI Bhaumik Bhatt
2020-07-24 4:11 ` kernel test robot
2020-07-24 23:46 ` kernel test robot
2020-07-23 22:36 ` [PATCH v5 08/10] bus: mhi: core: Use counters to track MHI device state transitions Bhaumik Bhatt
2020-07-24 5:24 ` Manivannan Sadhasivam
2020-07-23 22:36 ` [PATCH v5 09/10] bus: mhi: core: Read and save device hardware information from BHI Bhaumik Bhatt
2020-07-23 22:36 ` [PATCH v5 10/10] bus: mhi: core: Introduce sysfs entries for MHI Bhaumik Bhatt
2020-07-24 5:42 ` Manivannan Sadhasivam
2020-07-28 0:29 ` bbhatt [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ebca0d6ccce616fec6fd0fc08230c19e@codeaurora.org \
--to=bbhatt@codeaurora.org \
--cc=hemantk@codeaurora.org \
--cc=jhugo@codeaurora.org \
--cc=linux-arm-msm-owner@vger.kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=manivannan.sadhasivam@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).