Re: bus/mhi/core: Double lock in mhi_device_put() and dev_wake inc/dec
On 2020-09-17 16:16, Shuah Khan wrote: While looking at this file for an unrelated issue, I happen to notice there is a double locking on mhi_cntrl->pm_lock in the mhi_device_put() when it gets called from mhi_driver_remove() The other two calls from mhi_driver_probe() don't hold the pm_lock. In addition, lock holding while dev_wake updates is inconsistent. dev_wake gets incremented and decremented without holding pm_lock in mhi_device_get(), mhi_device_get_sync() and mhi_device_put(). Exception are when mhi_device_put() is called from mhi_driver_remove(). The following commit is where all this code is added. bus: mhi: core: Add support for data transfer https://github.com/torvalds/linux/commit/189ff97cca53e3fe2d8b38d64105040ce17fc62d It appears to be real problem. I don't have a way to test this driver, hence reaching out to let you know about my findings. thanks, -- Shuah Thank you for inputs. Hemant and I discussed this and we agree that there are inconsistencies we need to fix. We will be uploading a patch to remove the read_lock_bh/read_unlock_bh calls from the mhi_driver_remove(). Thanks, Bhaumik 'The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project'
Re: [PATCH v1 1/3] bus: mhi: core: Remove warnings for missing MODULE_LICENSE()
On 2020-09-18 10:18, Manivannan Sadhasivam wrote:
On Fri, Sep 18, 2020 at 09:49:05AM -0700, bbh...@codeaurora.org wrote:
On 2020-09-18 07:27, Jeffrey Hugo wrote:
> On 9/17/2020 4:19 PM, Bhaumik Bhatt wrote:
> > When building MHI as a module, missing MODULE_LICENSE() warnings
> > are seen. Avoid them by adding the license and description
> > information for the files where the warnings are seen.
> >
> > Signed-off-by: Bhaumik Bhatt
> > ---
> > drivers/bus/mhi/core/boot.c | 3 +++
> > drivers/bus/mhi/core/main.c | 3 +++
> > drivers/bus/mhi/core/pm.c | 3 +++
> > 3 files changed, 9 insertions(+)
> >
> > diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
> > index 24422f5..78140cc 100644
> > --- a/drivers/bus/mhi/core/boot.c
> > +++ b/drivers/bus/mhi/core/boot.c
> > @@ -523,3 +523,6 @@ void mhi_fw_load_handler(struct mhi_controller
> > *mhi_cntrl)
> > error_alloc_fw_table:
> > release_firmware(firmware);
> > }
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("MHI Host Interface");
> > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > index 2cff5dd..172026f 100644
> > --- a/drivers/bus/mhi/core/main.c
> > +++ b/drivers/bus/mhi/core/main.c
> > @@ -1533,3 +1533,6 @@ int mhi_poll(struct mhi_device *mhi_dev, u32
> > budget)
> > return ret;
> > }
> > EXPORT_SYMBOL_GPL(mhi_poll);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("MHI Host Interface");
> > diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> > index ce4d969..72c3dbc 100644
> > --- a/drivers/bus/mhi/core/pm.c
> > +++ b/drivers/bus/mhi/core/pm.c
> > @@ -1150,3 +1150,6 @@ void mhi_device_put(struct mhi_device *mhi_dev)
> > read_unlock_bh(_cntrl->pm_lock);
> > }
> > EXPORT_SYMBOL_GPL(mhi_device_put);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("MHI Host Interface");
> >
>
> I would expect you only need to add the MODULE_* once per module, in
> which case main.c is probably the only place that needs it.
Hi Jeff,
I thought so too. This is to fix below warnings seen when building MHI
as a
MODULE:
WARNING: modpost: missing MODULE_LICENSE() in
drivers/bus/mhi/core/main.o
WARNING: modpost: missing MODULE_LICENSE() in
drivers/bus/mhi/core/pm.o
WARNING: modpost: missing MODULE_LICENSE() in
drivers/bus/mhi/core/boot.o
We've only had those in init.c so far.
Can you please test below diff to see if it fixes the warning?
diff --git a/drivers/bus/mhi/core/Makefile
b/drivers/bus/mhi/core/Makefile
index 66e2700c9032..bc1469778cf8 100644
--- a/drivers/bus/mhi/core/Makefile
+++ b/drivers/bus/mhi/core/Makefile
@@ -1,3 +1,3 @@
-obj-$(CONFIG_MHI_BUS) := mhi.o
+obj-$(CONFIG_MHI_BUS) += mhi.o
mhi-y := init.o main.o pm.o boot.o
Thanks,
Mani
Thanks,
Bhaumik
'The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,\na Linux Foundation Collaborative Project'
Hi Mani,
Yes I was just about to reply. I realized it was due to the Makefile
change. I have fixed and
tested it. The warnings are gone now. I will remove the patch.
Thanks,
Bhaumik
'The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,\na Linux Foundation Collaborative Project'
Re: [PATCH v1 2/3] bus: mhi: core: Introduce debugfs entries for MHI
On 2020-09-17 16:36, Randy Dunlap wrote: On 9/17/20 3:19 PM, Bhaumik Bhatt wrote: diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig index a8bd9bd..ae68347 100644 --- a/drivers/bus/mhi/Kconfig +++ b/drivers/bus/mhi/Kconfig @@ -12,3 +12,11 @@ config MHI_BUS communication protocol used by the host processors to control and communicate with modem devices over a high speed peripheral bus or shared memory. + +config MHI_BUS_DEBUG + bool "Debugfs support for the MHI bus" + depends on MHI_BUS && DEBUG_FS + help +Enable debugfs support for use with the MHI transport. Allows +reading and/or modifying some values within the MHI controller +for debug and test purposes. from Documentation/process/coding-style.rst: """For all of the Kconfig* configuration files throughout the source tree, the indentation is somewhat different. Lines under a ``config`` definition are indented with one tab, while help text is indented an additional two spaces.""" Several lines above use spaces instead of one tab... Thank you for pointing out. I will fix this.
Re: [PATCH v1 1/3] bus: mhi: core: Remove warnings for missing MODULE_LICENSE()
On 2020-09-18 07:27, Jeffrey Hugo wrote:
On 9/17/2020 4:19 PM, Bhaumik Bhatt wrote:
When building MHI as a module, missing MODULE_LICENSE() warnings
are seen. Avoid them by adding the license and description
information for the files where the warnings are seen.
Signed-off-by: Bhaumik Bhatt
---
drivers/bus/mhi/core/boot.c | 3 +++
drivers/bus/mhi/core/main.c | 3 +++
drivers/bus/mhi/core/pm.c | 3 +++
3 files changed, 9 insertions(+)
diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
index 24422f5..78140cc 100644
--- a/drivers/bus/mhi/core/boot.c
+++ b/drivers/bus/mhi/core/boot.c
@@ -523,3 +523,6 @@ void mhi_fw_load_handler(struct mhi_controller
*mhi_cntrl)
error_alloc_fw_table:
release_firmware(firmware);
}
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MHI Host Interface");
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 2cff5dd..172026f 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -1533,3 +1533,6 @@ int mhi_poll(struct mhi_device *mhi_dev, u32
budget)
return ret;
}
EXPORT_SYMBOL_GPL(mhi_poll);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MHI Host Interface");
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index ce4d969..72c3dbc 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -1150,3 +1150,6 @@ void mhi_device_put(struct mhi_device *mhi_dev)
read_unlock_bh(_cntrl->pm_lock);
}
EXPORT_SYMBOL_GPL(mhi_device_put);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MHI Host Interface");
I would expect you only need to add the MODULE_* once per module, in
which case main.c is probably the only place that needs it.
Hi Jeff,
I thought so too. This is to fix below warnings seen when building MHI
as a MODULE:
WARNING: modpost: missing MODULE_LICENSE() in
drivers/bus/mhi/core/main.o
WARNING: modpost: missing MODULE_LICENSE() in drivers/bus/mhi/core/pm.o
WARNING: modpost: missing MODULE_LICENSE() in
drivers/bus/mhi/core/boot.o
We've only had those in init.c so far.
Thanks,
Bhaumik
'The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,\na Linux Foundation Collaborative Project'
Re: [PATCH v7 00/11] Introduce features and debugfs/sysfs entries for MHI
On 2020-08-10 23:26, Manivannan Sadhasivam wrote: Hi Bhaumik, On Mon, Aug 10, 2020 at 03:00:54PM -0700, Bhaumik Bhatt wrote: 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. Sorry for stretching the review so long. Will apply the series to mhi-next once v5.9-rc1 is out. Thanks, Mani Awesome. Thank you for the thorough review. v7: -Added suggested-by and reviewed-by tags -Fixed nitpick on removal of M3_fast counter as it was unused -Updated sysfs documentation dates and intended kernel version -Fixed minor debugfs formatting by removing an extra newline character v6: -Introduced APIs for allocating and freeing the MHI controller so as to ensure that it is always zero-initialized -Moved gerrits around for counter introduction -Fixed documentation for sysfs 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 (11): 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 counters to track MHI device state transitions bus: mhi: core: Introduce debugfs entries for MHI bus: mhi: core: Read and save device hardware information from BHI bus: mhi: core: Introduce APIs to allocate and free the MHI controller bus: mhi: core: Introduce sysfs entries for MHI Documentation/ABI/stable/sysfs-bus-mhi | 21 ++ 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| 81 ++- 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| 30 ++- 11 files changed, 623 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
Re: [PATCH v6 11/11] bus: mhi: core: Introduce sysfs entries for MHI
On 2020-08-06 22:22, Manivannan Sadhasivam wrote:
On Mon, Jul 27, 2020 at 07:02:20PM -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
---
Documentation/ABI/stable/sysfs-bus-mhi | 21 ++
MAINTAINERS| 1 +
drivers/bus/mhi/core/init.c| 53
++
3 files changed, 75 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 000..1d5d0d6
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-bus-mhi
@@ -0,0 +1,21 @@
+What: /sys/bus/mhi/devices/.../serialnumber
+Date: Jul 2020
+KernelVersion: 5.8
+Contact: Bhaumik Bhatt
+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 device
info.
I think you're not using tabs here and that's why it is showing
mangled. Please
use tabs as like other files.
Thanks,
Mani
Hi Mani,
I am using tabs actually. I, in fact, copied another file
(sysfs-bus-vmbus) and only modified the
required entries and did a diff to confirm.
I doubt there is more I can do.
Please let me know if the next patch is acceptable soon.
+
+What: /sys/bus/mhi/devices/.../oem_pk_hash
+Date: Jul 2020
+KernelVersion: 5.8
+Contact: Bhaumik Bhatt
+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 device
info.
diff --git a/MAINTAINERS b/MAINTAINERS
index e64e5db..5e49316 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11018,6 +11018,7 @@ M: Hemant Kumar
L: linux-arm-...@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 972dbf0..c086ef2 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[] = {
+ _attr_serial_number.attr,
+ _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(_cntrl->mhi_dev->dev.kobj,
+ _sysfs_group);
+}
+
+static void mhi_destroy_sysfs(struct mhi_controller *mhi_cntrl)
+{
+ sysfs_remove_group(_cntrl->mhi_dev->dev.kobj, _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,
Re: [PATCH v5 10/10] bus: mhi: core: Introduce sysfs entries for MHI
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
---
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 000..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
+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
+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
L: linux-arm-...@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,
+
Re: [PATCH v4 7/9] bus: mhi: core: Introduce debugfs entries and counters for MHI
On 2020-07-04 08:41, Manivannan Sadhasivam wrote:
On Mon, Jun 29, 2020 at 09:39:40AM -0700, Bhaumik Bhatt wrote:
Introduce debugfs entries to show state, register, channel, and event
ring information. Add MHI state counters to keep track of the state
changes on the device. Also, allow the host to trigger a device reset,
issue votes, and change the MHI timeout to help in debug.
Signed-off-by: Bhaumik Bhatt
---
drivers/bus/mhi/Kconfig | 8 +
drivers/bus/mhi/core/Makefile | 5 +-
drivers/bus/mhi/core/debugfs.c | 444
drivers/bus/mhi/core/init.c | 7 +
drivers/bus/mhi/core/internal.h | 24 +++
drivers/bus/mhi/core/pm.c | 4 +
include/linux/mhi.h | 4 +
7 files changed, 493 insertions(+), 3 deletions(-)
create mode 100644 drivers/bus/mhi/core/debugfs.c
diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig
index a8bd9bd..6a217ff 100644
--- a/drivers/bus/mhi/Kconfig
+++ b/drivers/bus/mhi/Kconfig
@@ -12,3 +12,11 @@ config MHI_BUS
communication protocol used by the host processors to control
and communicate with modem devices over a high speed peripheral
bus or shared memory.
+
+config MHI_BUS_DEBUG
+ bool "Debugfs support for the MHI bus"
+ depends on MHI_BUS && DEBUG_FS
+ help
+Enable debugfs support for use with the MHI transport. Allows
+reading and/or modifying some values within the MHI controller
+for debug and test purposes.
diff --git a/drivers/bus/mhi/core/Makefile
b/drivers/bus/mhi/core/Makefile
index 66e2700..460a548 100644
--- a/drivers/bus/mhi/core/Makefile
+++ b/drivers/bus/mhi/core/Makefile
@@ -1,3 +1,2 @@
-obj-$(CONFIG_MHI_BUS) := mhi.o
-
-mhi-y := init.o main.o pm.o boot.o
+obj-$(CONFIG_MHI_BUS) := init.o main.o pm.o boot.o
+obj-$(CONFIG_MHI_BUS_DEBUG) += debugfs.o
diff --git a/drivers/bus/mhi/core/debugfs.c
b/drivers/bus/mhi/core/debugfs.c
new file mode 100644
index 000..266cbf0
--- /dev/null
+++ b/drivers/bus/mhi/core/debugfs.c
@@ -0,0 +1,444 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include "internal.h"
+
+static int mhi_debugfs_states_show(struct seq_file *m, void *d)
+{
+ struct mhi_controller *mhi_cntrl = m->private;
+
+ /* states */
+ seq_printf(m, "PM state:%s Device:%s MHI state:%s EE:%s wake:%s\n",
+ to_mhi_pm_state_str(mhi_cntrl->pm_state),
+ mhi_is_active(mhi_cntrl) ? "Active" : "Inactive",
+ TO_MHI_STATE_STR(mhi_cntrl->dev_state),
+ TO_MHI_EXEC_STR(mhi_cntrl->ee),
+ mhi_cntrl->wake_set ? "true" : "false");
Nit: Always use a space after ":".
Sure.
+
+ /* counters */
+ seq_printf(m, "M0:%u M2:%u M3:%u M3_Fast:%u", mhi_cntrl->M0,
+ mhi_cntrl->M2, mhi_cntrl->M3, mhi_cntrl->M3_fast);
+
+ seq_printf(m, " device wake:%u pending packets:%u\n",
+ atomic_read(_cntrl->dev_wake),
+ atomic_read(_cntrl->pending_pkts));
+
+ return 0;
+}
+
+static int mhi_debugfs_events_show(struct seq_file *m, void *d)
+{
+ struct mhi_controller *mhi_cntrl = m->private;
+ struct mhi_event *mhi_event;
+ struct mhi_event_ctxt *er_ctxt;
+ int i;
+
+ if (!mhi_is_active(mhi_cntrl)) {
+ seq_puts(m, "Device not ready\n");
+ return -ENODEV;
+ }
+
+ er_ctxt = mhi_cntrl->mhi_ctxt->er_ctxt;
+ mhi_event = mhi_cntrl->mhi_event;
+ for (i = 0; i < mhi_cntrl->total_ev_rings;
+ i++, er_ctxt++, mhi_event++) {
+ struct mhi_ring *ring = _event->ring;
+
+ if (mhi_event->offload_ev) {
+ seq_printf(m, "Index:%d is an offload event ring\n", i);
+ continue;
+ }
+
+ seq_printf(m, "Index:%d intmod count:%lu time:%lu",
+ i, (er_ctxt->intmod & EV_CTX_INTMODC_MASK) >>
+ EV_CTX_INTMODC_SHIFT,
+ (er_ctxt->intmod & EV_CTX_INTMODT_MASK) >>
+ EV_CTX_INTMODT_SHIFT);
+
+ seq_printf(m, " base:0x%0llx len:0x%llx", er_ctxt->rbase,
+ er_ctxt->rlen);
+
+ seq_printf(m,
+ " rp:0x%llx wp:0x%llx local rp:0x%llx db:0x%llx\n",
+ er_ctxt->rp, er_ctxt->wp, (u64)ring->rp,
+ mhi_event->db_cfg.db_val);
+ }
+
+ return 0;
+}
+
+static int mhi_debugfs_channels_show(struct seq_file *m, void *d)
+{
+ struct mhi_controller *mhi_cntrl = m->private;
+ struct mhi_chan *mhi_chan;
+ struct mhi_chan_ctxt *chan_ctxt;
+ int i;
+
+ if (!mhi_is_active(mhi_cntrl)) {
+ seq_puts(m, "Device
Re: [PATCH v4 3/9] bus: mhi: core: Use helper API to trigger a non-blocking host resume
On 2020-07-04 07:47, Manivannan Sadhasivam wrote:
On Mon, Jun 29, 2020 at 09:39:36AM -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.
Why can't you use pm_wakeup_event() as done in __mhi_device_get_sync()?
Thanks,
Mani
I forgot to address the __mhi_device_get_sync() function. Thanks for
pointing out.
Is it preferable to always post wakeup source with hard boolean set?
We do want to wakeup from Suspend-to-Idle if system suspend happens to
go that route.
As of now, we just by default do regular wakeup event and not hard.
I figured at some point we might need to distinguish between hard vs
regular, hence the option but
it can be eliminated in favor of one or another.
Thanks,
Bhaumik
Signed-off-by: Bhaumik Bhatt
---
drivers/bus/mhi/core/internal.h | 8
drivers/bus/mhi/core/main.c | 21 +++--
drivers/bus/mhi/core/pm.c | 6 ++
3 files changed, 17 insertions(+), 18 deletions(-)
diff --git a/drivers/bus/mhi/core/internal.h
b/drivers/bus/mhi/core/internal.h
index bcfa7b6..cb32eaf 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -599,6 +599,14 @@ int mhi_queue_state_transition(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,
+ bool hard_wakeup)
+{
+ pm_wakeup_dev_event(_cntrl->mhi_dev->dev, 0, hard_wakeup);
+ 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..8d6ec34 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, false);
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, false);
/* 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, false);
/* 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(_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, false);
/* 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..5e3994e 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -1139,10 +1139,8 @@ void mhi_device_put(struct mhi_device *mhi_dev)
mhi_dev->dev_wake--;
read_lock_bh(_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, false);
mhi_cntrl->wake_put(mhi_cntrl, false);
Re: [PATCH v3 0/7] Introduce features and debugfs/sysfs entries for MHI
On 2020-05-21 06:23, Manivannan Sadhasivam wrote: Hi, On Mon, May 18, 2020 at 01:03:54PM -0700, Bhaumik Bhatt wrote: Introduce independent bus and device voting mechanism for clients and 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. I think this series is not yet ready. So will not merge any patches in this series for 5.8. Or let me know if there are any independent patches which should get merged. I'm planning to send the final 5.8 series to Greg by Friday. Thanks, Mani This set of patches was tested on arm64 and x86. 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 (7): bus: mhi: core: Abort suspends due to outgoing pending packets bus: mhi: core: Introduce independent voting mechanism 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 and counters for MHI 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 | 501 + drivers/bus/mhi/core/init.c| 80 +- drivers/bus/mhi/core/internal.h| 29 ++ drivers/bus/mhi/core/main.c| 6 +- drivers/bus/mhi/core/pm.c | 79 -- include/linux/mhi.h| 39 ++- 11 files changed, 745 insertions(+), 45 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 Hi Mani, These are patches already reviewed, small and important and good to go: bus: mhi: core: Abort suspends due to outgoing pending packets bus: mhi: core: Use generic name field for an MHI device bus: mhi: core: Introduce helper function to check device state bus: mhi: core: Read and save device hardware information from BHI Please let us know if you have any concerns. Thanks, Bhaumik
Re: [PATCH v3 2/7] bus: mhi: core: Introduce independent voting mechanism
On 2020-05-20 12:06, Jeffrey Hugo wrote: On 5/20/2020 12:43 PM, bbh...@codeaurora.org wrote: On 2020-05-20 09:54, Jeffrey Hugo wrote: On 5/18/2020 2:03 PM, Bhaumik Bhatt wrote: Allow independent votes from clients such that they can choose to vote for either the device or the bus or both. This helps in cases where the device supports autonomous low power mode wherein it can move to M2 state without the need to notify the host. Clients can also vote only to keep the underlying bus active without having the device in M0 state to support offload use cases. Signed-off-by: Bhaumik Bhatt --- I wonder, why doesn't this fit with runtimePM? Hi Jeff, Can you elaborate? In short, with this patch, MHI just wants to give controller the option to choose the vote type so we can implement autonomous low power mode entries on both host and device. So, you are attempting to manage the power mode of the device. The standard mechanism to do so in Linux is runtime pm. https://elixir.bootlin.com/linux/latest/source/Documentation/driver-api/pm/devices.rst I'm no runtime pm expert, but it feels like your whole voting mechanism, etc is just reimplemeting that. Reimplementing the wheel, when its been a standard thing that the majority of the kernel uses is not usually acceptable. IMO, you need some sort of justification why runtime pm is not applicable for you, because I'm willing to bet Mani/Greg are going to ask the same. I think we can look at the patch as simply expanding the scope of what already exists. The client here has been calling mhi_device_get/put/sync APIs to gain device vote and with new features yet to come in, this introductory change is only re-purposing what voting means going forward. i.e. allowing individual bus and device votes. If you're suggesting using runtimePM APIs to replace the newly introduced bus vote, it would be kind of overkill here IMO. Is that what you were getting at? Because currently, we just have controllers use runtimePM and provide callbacks to them. If you have ideas, we can discuss them.
Re: [PATCH v3 2/7] bus: mhi: core: Introduce independent voting mechanism
On 2020-05-20 09:54, Jeffrey Hugo wrote: On 5/18/2020 2:03 PM, Bhaumik Bhatt wrote: Allow independent votes from clients such that they can choose to vote for either the device or the bus or both. This helps in cases where the device supports autonomous low power mode wherein it can move to M2 state without the need to notify the host. Clients can also vote only to keep the underlying bus active without having the device in M0 state to support offload use cases. Signed-off-by: Bhaumik Bhatt --- I wonder, why doesn't this fit with runtimePM? Hi Jeff, Can you elaborate? In short, with this patch, MHI just wants to give controller the option to choose the vote type so we can implement autonomous low power mode entries on both host and device. Let us know if you need some more information. Thanks, Bhaumik
Re: [PATCH] bus: mhi: core: Use current ee in intvec handler
On 2020-05-17 12:38, Jeffrey Hugo wrote:
On 5/15/2020 8:58 PM, bbh...@codeaurora.org wrote:
On 2020-05-14 19:17, Jeffrey Hugo wrote:
The intvec handler stores the caches ee in a local variable for use
in
processing the intvec. It should instead use the current ee which is
read at the beginning of the intvec incase that the intvec is related
to
an ee change. Otherwise, the handler might make the wrong decision
based on an incorrect ee.
Fixes: 3000f85b8f47 (bus: mhi: core: Add support for basic PM
operations)
Signed-off-by: Jeffrey Hugo
---
drivers/bus/mhi/core/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/bus/mhi/core/main.c
b/drivers/bus/mhi/core/main.c
index 7272a5a..0a41fe5 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -386,8 +386,8 @@ irqreturn_t mhi_intvec_threaded_handler(int
irq_number, void *dev)
write_lock_irq(_cntrl->pm_lock);
if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
state = mhi_get_mhi_state(mhi_cntrl);
- ee = mhi_cntrl->ee;
mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
+ ee = mhi_cntrl->ee;
}
if (state == MHI_STATE_SYS_ERR) {
Hi Jeff,
Let's hold off on this change for now please as we have some good set
of
bug fixes and improvements coming in very soon. They're only pending
post
to LKML.
Does that series of changes address the same issue this patch does,
and are they going to be posted soon (ie this week)?
Yes.
Re: [PATCH] bus: mhi: core: Use current ee in intvec handler
On 2020-05-14 19:17, Jeffrey Hugo wrote:
The intvec handler stores the caches ee in a local variable for use in
processing the intvec. It should instead use the current ee which is
read at the beginning of the intvec incase that the intvec is related
to
an ee change. Otherwise, the handler might make the wrong decision
based on an incorrect ee.
Fixes: 3000f85b8f47 (bus: mhi: core: Add support for basic PM
operations)
Signed-off-by: Jeffrey Hugo
---
drivers/bus/mhi/core/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 7272a5a..0a41fe5 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -386,8 +386,8 @@ irqreturn_t mhi_intvec_threaded_handler(int
irq_number, void *dev)
write_lock_irq(_cntrl->pm_lock);
if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
state = mhi_get_mhi_state(mhi_cntrl);
- ee = mhi_cntrl->ee;
mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
+ ee = mhi_cntrl->ee;
}
if (state == MHI_STATE_SYS_ERR) {
Hi Jeff,
Let's hold off on this change for now please as we have some good set of
bug fixes and improvements coming in very soon. They're only pending
post
to LKML.
Thanks
Re: [PATCH v5 8/8] bus: mhi: core: Ensure non-zero session or sequence ID values are used
On 2020-05-05 08:57, Jeffrey Hugo wrote:
On 5/4/2020 8:44 PM, Bhaumik Bhatt wrote:
While writing any sequence or session identifiers, it is possible that
the host could write a zero value, whereas only non-zero values should
be supported writes to those registers. Ensure that the host does not
write a non-zero value for them and also log them in debug messages.
Signed-off-by: Bhaumik Bhatt
---
drivers/bus/mhi/core/boot.c | 15 +++
drivers/bus/mhi/core/internal.h | 2 ++
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
index e5fcde1..7b9b561 100644
--- a/drivers/bus/mhi/core/boot.c
+++ b/drivers/bus/mhi/core/boot.c
@@ -43,10 +43,7 @@ void mhi_rddm_prepare(struct mhi_controller
*mhi_cntrl,
lower_32_bits(mhi_buf->dma_addr));
mhi_write_reg(mhi_cntrl, base, BHIE_RXVECSIZE_OFFS,
mhi_buf->len);
- sequence_id = prandom_u32() & BHIE_RXVECSTATUS_SEQNUM_BMSK;
-
- if (unlikely(!sequence_id))
- sequence_id = 1;
+ sequence_id = MHI_RANDOM_U32_NONZERO(BHIE_RXVECSTATUS_SEQNUM_BMSK);
mhi_write_reg_field(mhi_cntrl, base, BHIE_RXVECDB_OFFS,
BHIE_RXVECDB_SEQNUM_BMSK, BHIE_RXVECDB_SEQNUM_SHFT,
@@ -189,7 +186,9 @@ static int mhi_fw_load_amss(struct mhi_controller
*mhi_cntrl,
return -EIO;
}
- dev_dbg(dev, "Starting AMSS download via BHIe\n");
+ sequence_id = MHI_RANDOM_U32_NONZERO(BHIE_TXVECSTATUS_SEQNUM_BMSK);
+ dev_dbg(dev, "Starting AMSS download via BHIe. Sequence ID:%u\n",
+ sequence_id);
mhi_write_reg(mhi_cntrl, base, BHIE_TXVECADDR_HIGH_OFFS,
upper_32_bits(mhi_buf->dma_addr));
@@ -198,7 +197,6 @@ static int mhi_fw_load_amss(struct
mhi_controller *mhi_cntrl,
mhi_write_reg(mhi_cntrl, base, BHIE_TXVECSIZE_OFFS,
mhi_buf->len);
- sequence_id = prandom_u32() & BHIE_TXVECSTATUS_SEQNUM_BMSK;
mhi_write_reg_field(mhi_cntrl, base, BHIE_TXVECDB_OFFS,
BHIE_TXVECDB_SEQNUM_BMSK, BHIE_TXVECDB_SEQNUM_SHFT,
sequence_id);
@@ -246,14 +244,15 @@ static int mhi_fw_load_sbl(struct mhi_controller
*mhi_cntrl,
goto invalid_pm_state;
}
- dev_dbg(dev, "Starting SBL download via BHI\n");
+ session_id = MHI_RANDOM_U32_NONZERO(BHI_TXDB_SEQNUM_BMSK);
+ dev_dbg(dev, "Starting SBL download via BHI. Session ID:%u\n",
+ session_id);
mhi_write_reg(mhi_cntrl, base, BHI_STATUS, 0);
mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_HIGH,
upper_32_bits(dma_addr));
mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_LOW,
lower_32_bits(dma_addr));
mhi_write_reg(mhi_cntrl, base, BHI_IMGSIZE, size);
- session_id = prandom_u32() & BHI_TXDB_SEQNUM_BMSK;
mhi_write_reg(mhi_cntrl, base, BHI_IMGTXDB, session_id);
read_unlock_bh(pm_lock);
diff --git a/drivers/bus/mhi/core/internal.h
b/drivers/bus/mhi/core/internal.h
index 0965ca3..3205a92 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -452,6 +452,8 @@ enum mhi_pm_state {
#define PRIMARY_CMD_RING 0
#define MHI_DEV_WAKE_DB 127
#define MHI_MAX_MTU 0x
+#define MHI_RANDOM_U32_NONZERO(bmsk) ((prandom_u32_max(U32_MAX - 1) &
\
+(bmsk)) + 1)
I still think this is broken. I'm sorry for the back and forth.
So, again if prandom_u32_max returns 0xFF, and bmsk is 0xF, we get 0xF
by the & operation, then we add 1, which makes the result 0x10, which
is outside of the range of bmsk, and is basically 0, assuming the
register doesn't accept values outside of the lower 4 bits.
I think the solution should be:
prandom_u32_max(bmsk) + 1
If we treat bmsk like a ordinary value (say 7), then prandom_u32_max
will return a value from 0-6. Then by adding 1, we shift that range
to 1-7, which I think is exactly what we want.
Now, this assumes that bmsk is a contiguous mask of bits from bit 0 to
N. IE 0xFF and 0x4F are valid, but 0xFB is not. Do you think that is
a valid assumption?
I was under the impression that prandom_u32_max will return a value
between 0 to
whatever is supplied (in your example 7) and not 6. I noticed the
description has
the round bracket to indicate that it is excluded.
If there is no need to do a bmsk - 1 then what you said makes sense.
Main thing is to not go above the mask and to get a random non-zero
value which
fits within the mask.
Re: [PATCH v4 8/8] bus: mhi: core: Ensure non-zero session or sequence ID values are used
On 2020-05-04 07:33, Jeffrey Hugo wrote: On 5/1/2020 8:32 PM, Bhaumik Bhatt wrote: While writing any sequence or session identifiers, it is possible that the host could write a zero value, whereas only non-zero values should be supported writes to those registers. Ensure that the host does not write a non-zero value for them and also log them in debug messages. Suggested-by: Jeffrey Hugo Signed-off-by: Bhaumik Bhatt --- drivers/bus/mhi/core/boot.c | 15 +++ drivers/bus/mhi/core/internal.h | 1 + 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c index e5fcde1..9fe9c59 100644 --- a/drivers/bus/mhi/core/boot.c +++ b/drivers/bus/mhi/core/boot.c @@ -43,10 +43,7 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl, lower_32_bits(mhi_buf->dma_addr)); mhi_write_reg(mhi_cntrl, base, BHIE_RXVECSIZE_OFFS, mhi_buf->len); - sequence_id = prandom_u32() & BHIE_RXVECSTATUS_SEQNUM_BMSK; - - if (unlikely(!sequence_id)) - sequence_id = 1; + sequence_id = (MHI_RANDOM_U32_NONZERO & BHIE_RXVECSTATUS_SEQNUM_BMSK); I don't think this math works. For example, if MHI_RANDOM_U32_NONZERO is 0x0FF0, and BHIE_RXVECSTATUS_SEQNUM_BMSK is 0xF, then sequence_id will end up being 0. In this case, SEQNUM BMSK is set to 0x3FFF so this change will still work as we only supplied a non-zero number macro to AND with the mask. However, I agree that may not be the case always that we would know the bitmask in advance so it is better to fix it for good. Thanks for the catch! I have updated the change to have the macro take the bitmask as a parameter and output a non-zero value.
Re: [PATCH v3 6/9] bus: mhi: core: WARN_ON for malformed vector table
On 2020-04-30 08:02, Jeffrey Hugo wrote:
On 4/29/2020 2:52 PM, Bhaumik Bhatt wrote:
Add a bounds check in the firmware copy routine to exit if a malformed
vector table is found while attempting to load the firmware in to the
BHIe vector table.
Signed-off-by: Bhaumik Bhatt
---
drivers/bus/mhi/core/boot.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
index 17c636b..bc70edc 100644
--- a/drivers/bus/mhi/core/boot.c
+++ b/drivers/bus/mhi/core/boot.c
@@ -362,8 +362,14 @@ static void mhi_firmware_copy(struct
mhi_controller *mhi_cntrl,
int i = 0;
struct mhi_buf *mhi_buf = img_info->mhi_buf;
struct bhi_vec_entry *bhi_vec = img_info->bhi_vec;
+ struct device *dev = _cntrl->mhi_dev->dev;
while (remainder) {
+ if (WARN_ON(i >= img_info->entries)) {
+ dev_err(dev, "Malformed vector table\n");
I feel like this message needs more detail. At a minimum, I think it
should indicate what vector table (BHIe). I think if you can identify
what file, etc the the glitch is in, that would be better. Maybe some
detail about i and img_info->entries.
If I see this error message, I should have enough information to
immediately debug the issue. If it tells enough to go directly into
the firmware file and have a look at entry X to see what might be
corrupt about it, that makes my debugging very efficient. If I have
to go back to the code to figure out what "Malformed vector table"
means, and then maybe apply a patch to get more data about the error -
the error message is not as useful as it should be.
I plan on dropping this patch as it looks like an unnecessary check
since we
will always rely on the firmware->size from the request_firmware() API
in order
to calculate the img_info->entries (or we can call it the number of
segments, in
our case). And we will never fail in the if loop as values will already
be
bounded.
+ return;
+ }
+
to_cpy = min(remainder, mhi_buf->len);
memcpy(mhi_buf->buf, buf, to_cpy);
bhi_vec->dma_addr = mhi_buf->dma_addr;
Re: [PATCH v3 9/9] bus: mhi: core: Ensure non-zero session or sequence ID values
On 2020-04-30 08:12, Jeffrey Hugo wrote: On 4/29/2020 2:52 PM, Bhaumik Bhatt wrote: While writing any sequence or session identifiers, it is possible that the host could write a zero value, whereas only non-zero values are supported writes to those registers. Ensure that host does not write a non-zero value for those cases. Signed-off-by: Bhaumik Bhatt --- drivers/bus/mhi/core/boot.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c index 0bc9c50..c9971d4 100644 --- a/drivers/bus/mhi/core/boot.c +++ b/drivers/bus/mhi/core/boot.c @@ -199,6 +199,9 @@ static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl, mhi_write_reg(mhi_cntrl, base, BHIE_TXVECSIZE_OFFS, mhi_buf->len); sequence_id = prandom_u32() & BHIE_TXVECSTATUS_SEQNUM_BMSK; + if (unlikely(!sequence_id)) + sequence_id = 1; Seems like you could use prandom_u32_max(), and add 1 to the result to eliminate the conditional. What do you think? Agreed. Done using an internal macro in those places. + mhi_write_reg_field(mhi_cntrl, base, BHIE_TXVECDB_OFFS, BHIE_TXVECDB_SEQNUM_BMSK, BHIE_TXVECDB_SEQNUM_SHFT, sequence_id); @@ -254,6 +257,9 @@ static int mhi_fw_load_sbl(struct mhi_controller *mhi_cntrl, lower_32_bits(dma_addr)); mhi_write_reg(mhi_cntrl, base, BHI_IMGSIZE, size); session_id = prandom_u32() & BHI_TXDB_SEQNUM_BMSK; + if (unlikely(!session_id)) + session_id = 1; + mhi_write_reg(mhi_cntrl, base, BHI_IMGTXDB, session_id); read_unlock_bh(pm_lock);
