Save hardware information from BHI.
Allow reading and modifying some MHI variables for debug, test, and
informational purposes using debugfs.
Read values for device specific hardware information to be used by OEMs in
factory testing such as serial number and PK hash using sysfs.
This set of patches was tested on arm64 and x86.
v5:
-Removed the debug entry to trigger reset and will be addressed in a seperate
patch
-Added patch bus: mhi: core: Use counters to track MHI device state transitions
-Updated helper API to trigger a non-blocking host resume
-Minor nitpicks also fixed
v4:
-Removed bus: mhi: core: Introduce independent voting mechanism patch
-Removed bus vote function from debugfs due to independent voting removal
-Added helper resume APIs to aid consolidation of spread out code
-Added a clean-up patch and a missing host resume in voting API
v3:
-Add patch to check for pending packets in suspend as a dependency for the
independent voting mechanism introduction
-Include register dump entry for debugfs to dump MHI, BHI, and BHIe registers
-Update commit message for the debugfs patch
-Updated Documentation/ABI with the required info for sysfs
-Updated debugfs patch to include a new KConfig entry and dependencies
-Updated reviewed-by for some patches
v2:
-Added a new debugfs.c file for specific debugfs entries and code
-Updated commit text and addressed some comments for voting change
-Made sure sysfs is only used for serial number and OEM PK hash usage
Bhaumik Bhatt (10):
bus: mhi: core: Remove double occurrence for mhi_ctrl_ev_task()
declaration
bus: mhi: core: Abort suspends due to outgoing pending packets
bus: mhi: core: Use helper API to trigger a non-blocking host resume
bus: mhi: core: Trigger host resume if suspended during
mhi_device_get()
bus: mhi: core: Use generic name field for an MHI device
bus: mhi: core: Introduce helper function to check device state
bus: mhi: core: Introduce debugfs entries for MHI
bus: mhi: core: Use counters to track MHI device state transitions
bus: mhi: core: Read and save device hardware information from BHI
bus: mhi: core: Introduce sysfs entries for MHI
Documentation/ABI/stable/sysfs-bus-mhi | 25 ++
MAINTAINERS | 1 +
drivers/bus/mhi/Kconfig | 8 +
drivers/bus/mhi/core/Makefile | 5 +-
drivers/bus/mhi/core/boot.c | 17 +-
drivers/bus/mhi/core/debugfs.c | 409 +++++++++++++++++++++++++++++++++
drivers/bus/mhi/core/init.c | 65 +++++-
drivers/bus/mhi/core/internal.h | 37 ++-
drivers/bus/mhi/core/main.c | 27 +--
drivers/bus/mhi/core/pm.c | 26 ++-
include/linux/mhi.h | 18 +-
11 files changed, 599 insertions(+), 39 deletions(-)
create mode 100644 Documentation/ABI/stable/sysfs-bus-mhi
create mode 100644 drivers/bus/mhi/core/debugfs.c
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Introduce a helper function to determine whether the device is in a
powered ON state and resides in one of the active MHI states. This will
allow for some use cases where access can be pre-determined.
Signed-off-by: Bhaumik Bhatt <[email protected]>
Reviewed-by: Jeffrey Hugo <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
---
drivers/bus/mhi/core/internal.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index 1bbd6e9..5a81a42 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -598,6 +598,11 @@ int mhi_pm_m3_transition(struct mhi_controller *mhi_cntrl);
int __mhi_device_get_sync(struct mhi_controller *mhi_cntrl);
int mhi_send_cmd(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
enum mhi_cmd_type cmd);
+static inline bool mhi_is_active(struct mhi_controller *mhi_cntrl)
+{
+ return (mhi_cntrl->dev_state >= MHI_STATE_M0 &&
+ mhi_cntrl->dev_state <= MHI_STATE_M3_FAST);
+}
static inline void mhi_trigger_resume(struct mhi_controller *mhi_cntrl)
{
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
mhi_ctrl_ev_task() in the internal header file occurred twice.
Remove one of the occurrences for clean-up.
Signed-off-by: Bhaumik Bhatt <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
---
drivers/bus/mhi/core/internal.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index b1f640b..bcfa7b6 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -592,7 +592,6 @@ void mhi_pm_st_worker(struct work_struct *work);
void mhi_pm_sys_err_handler(struct mhi_controller *mhi_cntrl);
void mhi_fw_load_worker(struct work_struct *work);
int mhi_ready_state_transition(struct mhi_controller *mhi_cntrl);
-void mhi_ctrl_ev_task(unsigned long data);
int mhi_pm_m0_transition(struct mhi_controller *mhi_cntrl);
void mhi_pm_m1_transition(struct mhi_controller *mhi_cntrl);
int mhi_pm_m3_transition(struct mhi_controller *mhi_cntrl);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Autonomous low power mode support requires the MHI host to resume from
multiple places and post a wakeup source to exit system suspend. This
needs to be done in a non-blocking manner. Introduce a helper API to
trigger the host resume for data transfers and other non-blocking use
cases while supporting implementation of autonomous low power modes.
Signed-off-by: Bhaumik Bhatt <[email protected]>
---
drivers/bus/mhi/core/internal.h | 7 +++++++
drivers/bus/mhi/core/main.c | 21 +++++++--------------
drivers/bus/mhi/core/pm.c | 13 ++++---------
3 files changed, 18 insertions(+), 23 deletions(-)
diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index bcfa7b6..1bbd6e9 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -599,6 +599,13 @@ int __mhi_device_get_sync(struct mhi_controller *mhi_cntrl);
int mhi_send_cmd(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
enum mhi_cmd_type cmd);
+static inline void mhi_trigger_resume(struct mhi_controller *mhi_cntrl)
+{
+ pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
+ mhi_cntrl->runtime_get(mhi_cntrl);
+ mhi_cntrl->runtime_put(mhi_cntrl);
+}
+
/* Register access methods */
void mhi_db_brstmode(struct mhi_controller *mhi_cntrl, struct db_cfg *db_cfg,
void __iomem *db_addr, dma_addr_t db_val);
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 1f622ce..79be18e 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -909,8 +909,7 @@ void mhi_ctrl_ev_task(unsigned long data)
* process it since we are probably in a suspended state,
* so trigger a resume.
*/
- mhi_cntrl->runtime_get(mhi_cntrl);
- mhi_cntrl->runtime_put(mhi_cntrl);
+ mhi_trigger_resume(mhi_cntrl);
return;
}
@@ -971,10 +970,8 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
}
/* we're in M3 or transitioning to M3 */
- if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
- mhi_cntrl->runtime_get(mhi_cntrl);
- mhi_cntrl->runtime_put(mhi_cntrl);
- }
+ if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
+ mhi_trigger_resume(mhi_cntrl);
/* Toggle wake to exit out of M2 */
mhi_cntrl->wake_toggle(mhi_cntrl);
@@ -1032,10 +1029,8 @@ int mhi_queue_dma(struct mhi_device *mhi_dev, enum dma_data_direction dir,
}
/* we're in M3 or transitioning to M3 */
- if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
- mhi_cntrl->runtime_get(mhi_cntrl);
- mhi_cntrl->runtime_put(mhi_cntrl);
- }
+ if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
+ mhi_trigger_resume(mhi_cntrl);
/* Toggle wake to exit out of M2 */
mhi_cntrl->wake_toggle(mhi_cntrl);
@@ -1147,10 +1142,8 @@ int mhi_queue_buf(struct mhi_device *mhi_dev, enum dma_data_direction dir,
read_lock_irqsave(&mhi_cntrl->pm_lock, flags);
/* we're in M3 or transitioning to M3 */
- if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
- mhi_cntrl->runtime_get(mhi_cntrl);
- mhi_cntrl->runtime_put(mhi_cntrl);
- }
+ if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
+ mhi_trigger_resume(mhi_cntrl);
/* Toggle wake to exit out of M2 */
mhi_cntrl->wake_toggle(mhi_cntrl);
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 661d704..b227d41 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -824,11 +824,8 @@ int __mhi_device_get_sync(struct mhi_controller *mhi_cntrl)
/* Wake up the device */
read_lock_bh(&mhi_cntrl->pm_lock);
mhi_cntrl->wake_get(mhi_cntrl, true);
- if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
- pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
- mhi_cntrl->runtime_get(mhi_cntrl);
- mhi_cntrl->runtime_put(mhi_cntrl);
- }
+ if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
+ mhi_trigger_resume(mhi_cntrl);
read_unlock_bh(&mhi_cntrl->pm_lock);
ret = wait_event_timeout(mhi_cntrl->state_event,
@@ -1139,10 +1136,8 @@ void mhi_device_put(struct mhi_device *mhi_dev)
mhi_dev->dev_wake--;
read_lock_bh(&mhi_cntrl->pm_lock);
- if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
- mhi_cntrl->runtime_get(mhi_cntrl);
- mhi_cntrl->runtime_put(mhi_cntrl);
- }
+ if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
+ mhi_trigger_resume(mhi_cntrl);
mhi_cntrl->wake_put(mhi_cntrl, false);
read_unlock_bh(&mhi_cntrl->pm_lock);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
---
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 <[email protected]>
+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.
+
+What: /sys/bus/mhi/devices/.../oem_pk_hash
+Date: July 2020
+KernelVersion: 5.8
+Contact: Bhaumik Bhatt <[email protected]>
+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 <[email protected]>
L: [email protected]
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)
+{
+ 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);
+}
+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
Use counters to track MHI device state transitions such as those
to M0, M2, or M3 states. This helps in better debug by allowing
the user to see the number of transitions to a certain state when
queried using the states debugfs entry.
Signed-off-by: Bhaumik Bhatt <[email protected]>
---
drivers/bus/mhi/core/pm.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 27bb471..ce4d969 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -256,6 +256,7 @@ int mhi_pm_m0_transition(struct mhi_controller *mhi_cntrl)
dev_err(dev, "Unable to transition to M0 state\n");
return -EIO;
}
+ mhi_cntrl->M0++;
/* Wake up the device */
read_lock_bh(&mhi_cntrl->pm_lock);
@@ -326,6 +327,8 @@ void mhi_pm_m1_transition(struct mhi_controller *mhi_cntrl)
mhi_cntrl->dev_state = MHI_STATE_M2;
write_unlock_irq(&mhi_cntrl->pm_lock);
+
+ mhi_cntrl->M2++;
wake_up_all(&mhi_cntrl->state_event);
/* If there are any pending resources, exit M2 immediately */
@@ -362,6 +365,7 @@ int mhi_pm_m3_transition(struct mhi_controller *mhi_cntrl)
return -EIO;
}
+ mhi_cntrl->M3++;
wake_up_all(&mhi_cntrl->state_event);
return 0;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Thu, Jul 23, 2020 at 03:36:40PM -0700, Bhaumik Bhatt wrote:
> Use counters to track MHI device state transitions such as those
> to M0, M2, or M3 states. This helps in better debug by allowing
> the user to see the number of transitions to a certain state when
> queried using the states debugfs entry.
>
> Signed-off-by: Bhaumik Bhatt <[email protected]>
This patch should come before the debugfs patch. Also the header addition
should be here.
Thanks,
Mani
> ---
> drivers/bus/mhi/core/pm.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 27bb471..ce4d969 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -256,6 +256,7 @@ int mhi_pm_m0_transition(struct mhi_controller *mhi_cntrl)
> dev_err(dev, "Unable to transition to M0 state\n");
> return -EIO;
> }
> + mhi_cntrl->M0++;
>
> /* Wake up the device */
> read_lock_bh(&mhi_cntrl->pm_lock);
> @@ -326,6 +327,8 @@ void mhi_pm_m1_transition(struct mhi_controller *mhi_cntrl)
> mhi_cntrl->dev_state = MHI_STATE_M2;
>
> write_unlock_irq(&mhi_cntrl->pm_lock);
> +
> + mhi_cntrl->M2++;
> wake_up_all(&mhi_cntrl->state_event);
>
> /* If there are any pending resources, exit M2 immediately */
> @@ -362,6 +365,7 @@ int mhi_pm_m3_transition(struct mhi_controller *mhi_cntrl)
> return -EIO;
> }
>
> + mhi_cntrl->M3++;
> wake_up_all(&mhi_cntrl->state_event);
>
> return 0;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
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 <[email protected]>
> ---
> 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 <[email protected]>
> +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.
> +
> +What: /sys/bus/mhi/devices/.../oem_pk_hash
> +Date: July 2020
> +KernelVersion: 5.8
> +Contact: Bhaumik Bhatt <[email protected]>
> +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 <[email protected]>
> L: [email protected]
> 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 '('.
> +{
> + 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,
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
>
On Thu, Jul 23, 2020 at 03:36:35PM -0700, Bhaumik Bhatt wrote:
> Autonomous low power mode support requires the MHI host to resume from
> multiple places and post a wakeup source to exit system suspend. This
> needs to be done in a non-blocking manner. Introduce a helper API to
> trigger the host resume for data transfers and other non-blocking use
> cases while supporting implementation of autonomous low power modes.
>
> Signed-off-by: Bhaumik Bhatt <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Thanks,
Mani
> ---
> drivers/bus/mhi/core/internal.h | 7 +++++++
> drivers/bus/mhi/core/main.c | 21 +++++++--------------
> drivers/bus/mhi/core/pm.c | 13 ++++---------
> 3 files changed, 18 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index bcfa7b6..1bbd6e9 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -599,6 +599,13 @@ int __mhi_device_get_sync(struct mhi_controller *mhi_cntrl);
> int mhi_send_cmd(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
> enum mhi_cmd_type cmd);
>
> +static inline void mhi_trigger_resume(struct mhi_controller *mhi_cntrl)
> +{
> + pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
> + mhi_cntrl->runtime_get(mhi_cntrl);
> + mhi_cntrl->runtime_put(mhi_cntrl);
> +}
> +
> /* Register access methods */
> void mhi_db_brstmode(struct mhi_controller *mhi_cntrl, struct db_cfg *db_cfg,
> void __iomem *db_addr, dma_addr_t db_val);
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 1f622ce..79be18e 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -909,8 +909,7 @@ void mhi_ctrl_ev_task(unsigned long data)
> * process it since we are probably in a suspended state,
> * so trigger a resume.
> */
> - mhi_cntrl->runtime_get(mhi_cntrl);
> - mhi_cntrl->runtime_put(mhi_cntrl);
> + mhi_trigger_resume(mhi_cntrl);
>
> return;
> }
> @@ -971,10 +970,8 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
> }
>
> /* we're in M3 or transitioning to M3 */
> - if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
> - mhi_cntrl->runtime_get(mhi_cntrl);
> - mhi_cntrl->runtime_put(mhi_cntrl);
> - }
> + if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
> + mhi_trigger_resume(mhi_cntrl);
>
> /* Toggle wake to exit out of M2 */
> mhi_cntrl->wake_toggle(mhi_cntrl);
> @@ -1032,10 +1029,8 @@ int mhi_queue_dma(struct mhi_device *mhi_dev, enum dma_data_direction dir,
> }
>
> /* we're in M3 or transitioning to M3 */
> - if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
> - mhi_cntrl->runtime_get(mhi_cntrl);
> - mhi_cntrl->runtime_put(mhi_cntrl);
> - }
> + if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
> + mhi_trigger_resume(mhi_cntrl);
>
> /* Toggle wake to exit out of M2 */
> mhi_cntrl->wake_toggle(mhi_cntrl);
> @@ -1147,10 +1142,8 @@ int mhi_queue_buf(struct mhi_device *mhi_dev, enum dma_data_direction dir,
> read_lock_irqsave(&mhi_cntrl->pm_lock, flags);
>
> /* we're in M3 or transitioning to M3 */
> - if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
> - mhi_cntrl->runtime_get(mhi_cntrl);
> - mhi_cntrl->runtime_put(mhi_cntrl);
> - }
> + if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
> + mhi_trigger_resume(mhi_cntrl);
>
> /* Toggle wake to exit out of M2 */
> mhi_cntrl->wake_toggle(mhi_cntrl);
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 661d704..b227d41 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -824,11 +824,8 @@ int __mhi_device_get_sync(struct mhi_controller *mhi_cntrl)
> /* Wake up the device */
> read_lock_bh(&mhi_cntrl->pm_lock);
> mhi_cntrl->wake_get(mhi_cntrl, true);
> - if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
> - pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
> - mhi_cntrl->runtime_get(mhi_cntrl);
> - mhi_cntrl->runtime_put(mhi_cntrl);
> - }
> + if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
> + mhi_trigger_resume(mhi_cntrl);
> read_unlock_bh(&mhi_cntrl->pm_lock);
>
> ret = wait_event_timeout(mhi_cntrl->state_event,
> @@ -1139,10 +1136,8 @@ void mhi_device_put(struct mhi_device *mhi_dev)
>
> mhi_dev->dev_wake--;
> read_lock_bh(&mhi_cntrl->pm_lock);
> - if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
> - mhi_cntrl->runtime_get(mhi_cntrl);
> - mhi_cntrl->runtime_put(mhi_cntrl);
> - }
> + if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
> + mhi_trigger_resume(mhi_cntrl);
>
> mhi_cntrl->wake_put(mhi_cntrl, false);
> read_unlock_bh(&mhi_cntrl->pm_lock);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
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 <[email protected]>
>> ---
>> 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 <[email protected]>
>> +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 <[email protected]>
>> +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 <[email protected]>
>> L: [email protected]
>> 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
>>