RE: [PATCH v7 4/7] i3c: master: add mastership handover support
>Those waits should be done in the master driver. Pass a timeout to
>->request_master() or make it a property of the i3c_master_controller
>if you like, but don't poll the status from the core.
Ok, I will move these pollings, check master has DA and MR done to
master driver method request_mastership. Then we will not need
check_events method in master driver. Every master driver can
handle mastership request process in request_mastership method.
>> +if (!ret)
>> +m->bus.cur_master = m->this;
>> +
>> +mr_req_done:
>> +i3c_bus_maintenance_unlock(>bus);
>> +i3c_bus_normaluse_lock(>bus);
>
>You should downgrade the lock instead of releasing it. I really need to
>get my head around this locking scheme because I'm pretty sure we had
>good reasons for the locking/unlocking dance Przemek had in his series.
That locking/unlocking dance was apparently because caller of
acquire_bus holds read lock to avoid device on which it is
operating does not disappear or get modified due to
maintenance activities happening inside maintenance (write) lock.
Hence, here first we unlock normaluse lock and then gets
maintenance lock.
Przemek: Was there any other reason behind that ?
Yes, I agree it is safer to combine maintenance (write) unlock and then
normaluse (read) lock, into downgrading maintenance lock to normaluse
lock in single step using downgrade_lock. I will make that change.
>> +
>> +if (!master->secondary)
>> +master->bus.cur_master = master->this;
>
>This change doesn't seem related to this patch. Looks like this should
>instead go to patch 3.
This is to initialize cur_master pointer in master object pointing to
I3C object for current master on the bus. Only main master is by default
current master at the beginning, so cur_master is initialized only
for the main master. And since set_info happens in bus_init during
master initialization this change is included in this patch.
>> +/*
>> + * Maintenance lock and master check above is used to
>> + * avoid race amongst devices sending MR requests
>> + * at the same time, as soon as ENEC MST is sent by the current
>> + * master. It ensure that only one MR request is processed,
>> + * rest MR requests on losing devices will timeout in wait MR
>> + * DONE state. And next MR requests are blocked due to DISEC MST
>> + * sent by current master in yield operation.
>> + * New master should send ENEC MST once it's work is done.
>> + * maintainanace lock is also needed for i3c_master_get_accmst_locked.
>> + */
>> +
>> +ret = i3c_master_disec_locked(m, I3C_BROADCAST_ADDR,
>> + I3C_CCC_EVENT_MR |
>> + I3C_CCC_EVENT_HJ);
>
>Isn't it taken care of at the controller level? I'm fine sending it
>here, but I suspect some controllers might actually auto-disable MRs
>once they receive one.
I think auto disable MR on receiving one, handles the case of multiple
devices sending MR requests at a same time. That is handled above using
maintenance lock and current master check.
DISEC MR, HJ is to avoid new master getting interrupted by MR or HJ
before it's use of bus is done. Once it's work is done ENEC MR, HJ is
sent using i3c_master_enable_mr_events(), may be we can
rename it to i3c_master_release_bus. It is not actual hand over of bus
but just allowing other master to acquire it.
I am not disabling MR and HJ at the hardware level. But I can add that
in acquire_bus as one more level of safety, if DISEC is disregarded for
some reason by any device.
> if (WARN_ON(m->this == m->bus.cur_master)) {
>
>And you should probably check that before send DISEC.
Sure, I will do that.
Regards,
Parshuram Thombare
Re: [PATCH v7 4/7] i3c: master: add mastership handover support
On Mon, 11 May 2020 15:14:49 +0200
Parshuram Thombare wrote:
> Added mastership acquire and yield functions.
>
> Signed-off-by: Parshuram Thombare
> ---
> drivers/i3c/master.c | 187 +++--
> include/linux/i3c/master.h | 23 +
> 2 files changed, 201 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 669bd7e45810..9c8250a6a2b0 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -16,6 +16,7 @@
> #include
> #include
> #include
> +#include
>
> #include "internals.h"
>
> @@ -467,6 +468,79 @@ static const char * const i3c_bus_mode_strings[] = {
> [I3C_BUS_MODE_MIXED_SLOW] = "mixed-slow",
> };
>
> +static int i3c_master_enable_mr_events(struct i3c_master_controller *master)
> +{
> + int ret;
> +
> + master->ops->enable_mr_events(master);
> + i3c_bus_maintenance_lock(>bus);
> + ret = i3c_master_enec_locked(master, I3C_BROADCAST_ADDR,
> + I3C_CCC_EVENT_MR | I3C_CCC_EVENT_HJ);
> + i3c_bus_maintenance_unlock(>bus);
> +
> + return ret;
> +}
> +
> +static int check_event_da_update(struct i3c_master_controller *m)
> +{
> + return m->ops->check_event_set(m, I3C_SLV_DA_UPDATE);
> +}
> +
> +static int check_event_mr_done(struct i3c_master_controller *m)
> +{
> + return m->ops->check_event_set(m, I3C_SLV_MR_DONE);
> +}
> +
> +/**
> + * i3c_master_acquire_bus() - acquire I3C bus mastership
> + * @m: I3C master object
> + *
> + * This function may sleep.
> + * It is expected to be called with normaluse_lock.
> + */
> +static int i3c_master_acquire_bus(struct i3c_master_controller *m)
> +{
> + int ret = 0;
> + u32 val;
> +
> + /*
> + * Request mastership needs maintenance(write) lock. So, to avoid
> + * deadlock, release normaluse(read) lock, which is expected to be
> + * held before calling this function.
> + * normaluse(read) lock is expected to be held before calling
> + * this function to avoid race with maintenance activities
> + * like DEFSLVS processing etc
> + */
> + i3c_bus_normaluse_unlock(>bus);
> + i3c_bus_maintenance_lock(>bus);
> +
> + if (m->this && m->this == m->bus.cur_master) {
> + i3c_master_disec_locked(m, I3C_BROADCAST_ADDR,
> + I3C_CCC_EVENT_MR |
> + I3C_CCC_EVENT_HJ);
> + goto mr_req_done;
> + }
> +
> + ret = readx_poll_timeout(check_event_da_update, m, val,
> + val, 100, 100);
> + if (ret)
> + goto mr_req_done;
> +
> + ret = m->ops->request_mastership(m);
> + if (ret)
> + goto mr_req_done;
> +
> + ret = readx_poll_timeout(check_event_mr_done, m, val,
> + val, 100, 100);
Those waits should be done in the master driver. Pass a timeout to
->request_master() or make it a property of the i3c_master_controller
if you like, but don't poll the status from the core.
> + if (!ret)
> + m->bus.cur_master = m->this;
> +
> +mr_req_done:
> + i3c_bus_maintenance_unlock(>bus);
> + i3c_bus_normaluse_lock(>bus);
You should downgrade the lock instead of releasing it. I really need to
get my head around this locking scheme because I'm pretty sure we had
good reasons for the locking/unlocking dance Przemek had in his series.
> + return ret;
> +}
> +
> static ssize_t mode_show(struct device *dev,
>struct device_attribute *da,
>char *buf)
> @@ -685,6 +759,33 @@ static int i3c_master_send_ccc_cmd_locked(struct
> i3c_master_controller *master,
> return 0;
> }
>
> +static int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
> + u8 addr)
> +{
> + struct i3c_ccc_getaccmst *accmst;
> + struct i3c_ccc_cmd_dest dest;
> + struct i3c_ccc_cmd cmd;
> + int ret;
> +
> + accmst = i3c_ccc_cmd_dest_init(, addr, sizeof(*accmst));
> + if (!accmst)
> + return -ENOMEM;
> +
> + i3c_ccc_cmd_init(, true, I3C_CCC_GETACCMST, , 1);
> +
> + ret = i3c_master_send_ccc_cmd_locked(master, );
> + if (ret)
> + goto out;
> +
> + if (dest.payload.len != sizeof(*accmst))
> + ret = -EIO;
> +
> +out:
> + i3c_ccc_cmd_dest_cleanup();
> +
> + return ret;
> +}
> +
> static struct i2c_dev_desc *
> i3c_master_find_i2c_dev_by_addr(const struct i3c_master_controller *master,
> u16 addr)
> @@ -1558,10 +1659,6 @@ int i3c_master_set_info(struct i3c_master_controller
> *master,
> if (!i3c_bus_dev_addr_is_avail(>bus, info->dyn_addr))
> return -EINVAL;
>
> - if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER &&
> - master->secondary)
> - return -EINVAL;
> -
> if (master->this)
> return
[PATCH v7 4/7] i3c: master: add mastership handover support
Added mastership acquire and yield functions.
Signed-off-by: Parshuram Thombare
---
drivers/i3c/master.c | 187 +++--
include/linux/i3c/master.h | 23 +
2 files changed, 201 insertions(+), 9 deletions(-)
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 669bd7e45810..9c8250a6a2b0 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#include "internals.h"
@@ -467,6 +468,79 @@ static const char * const i3c_bus_mode_strings[] = {
[I3C_BUS_MODE_MIXED_SLOW] = "mixed-slow",
};
+static int i3c_master_enable_mr_events(struct i3c_master_controller *master)
+{
+ int ret;
+
+ master->ops->enable_mr_events(master);
+ i3c_bus_maintenance_lock(>bus);
+ ret = i3c_master_enec_locked(master, I3C_BROADCAST_ADDR,
+I3C_CCC_EVENT_MR | I3C_CCC_EVENT_HJ);
+ i3c_bus_maintenance_unlock(>bus);
+
+ return ret;
+}
+
+static int check_event_da_update(struct i3c_master_controller *m)
+{
+ return m->ops->check_event_set(m, I3C_SLV_DA_UPDATE);
+}
+
+static int check_event_mr_done(struct i3c_master_controller *m)
+{
+ return m->ops->check_event_set(m, I3C_SLV_MR_DONE);
+}
+
+/**
+ * i3c_master_acquire_bus() - acquire I3C bus mastership
+ * @m: I3C master object
+ *
+ * This function may sleep.
+ * It is expected to be called with normaluse_lock.
+ */
+static int i3c_master_acquire_bus(struct i3c_master_controller *m)
+{
+ int ret = 0;
+ u32 val;
+
+ /*
+* Request mastership needs maintenance(write) lock. So, to avoid
+* deadlock, release normaluse(read) lock, which is expected to be
+* held before calling this function.
+* normaluse(read) lock is expected to be held before calling
+* this function to avoid race with maintenance activities
+* like DEFSLVS processing etc
+*/
+ i3c_bus_normaluse_unlock(>bus);
+ i3c_bus_maintenance_lock(>bus);
+
+ if (m->this && m->this == m->bus.cur_master) {
+ i3c_master_disec_locked(m, I3C_BROADCAST_ADDR,
+ I3C_CCC_EVENT_MR |
+ I3C_CCC_EVENT_HJ);
+ goto mr_req_done;
+ }
+
+ ret = readx_poll_timeout(check_event_da_update, m, val,
+val, 100, 100);
+ if (ret)
+ goto mr_req_done;
+
+ ret = m->ops->request_mastership(m);
+ if (ret)
+ goto mr_req_done;
+
+ ret = readx_poll_timeout(check_event_mr_done, m, val,
+val, 100, 100);
+ if (!ret)
+ m->bus.cur_master = m->this;
+
+mr_req_done:
+ i3c_bus_maintenance_unlock(>bus);
+ i3c_bus_normaluse_lock(>bus);
+ return ret;
+}
+
static ssize_t mode_show(struct device *dev,
struct device_attribute *da,
char *buf)
@@ -685,6 +759,33 @@ static int i3c_master_send_ccc_cmd_locked(struct
i3c_master_controller *master,
return 0;
}
+static int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
+ u8 addr)
+{
+ struct i3c_ccc_getaccmst *accmst;
+ struct i3c_ccc_cmd_dest dest;
+ struct i3c_ccc_cmd cmd;
+ int ret;
+
+ accmst = i3c_ccc_cmd_dest_init(, addr, sizeof(*accmst));
+ if (!accmst)
+ return -ENOMEM;
+
+ i3c_ccc_cmd_init(, true, I3C_CCC_GETACCMST, , 1);
+
+ ret = i3c_master_send_ccc_cmd_locked(master, );
+ if (ret)
+ goto out;
+
+ if (dest.payload.len != sizeof(*accmst))
+ ret = -EIO;
+
+out:
+ i3c_ccc_cmd_dest_cleanup();
+
+ return ret;
+}
+
static struct i2c_dev_desc *
i3c_master_find_i2c_dev_by_addr(const struct i3c_master_controller *master,
u16 addr)
@@ -1558,10 +1659,6 @@ int i3c_master_set_info(struct i3c_master_controller
*master,
if (!i3c_bus_dev_addr_is_avail(>bus, info->dyn_addr))
return -EINVAL;
- if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER &&
- master->secondary)
- return -EINVAL;
-
if (master->this)
return -EINVAL;
@@ -1570,7 +1667,9 @@ int i3c_master_set_info(struct i3c_master_controller
*master,
return PTR_ERR(i3cdev);
master->this = i3cdev;
- master->bus.cur_master = master->this;
+
+ if (!master->secondary)
+ master->bus.cur_master = master->this;
ret = i3c_master_attach_i3c_dev(master, i3cdev);
if (ret)
@@ -1612,6 +1711,73 @@ static void i3c_master_detach_free_devs(struct
i3c_master_controller *master)
}
}
+/**
+ * i3c_master_yield_bus() - yield I3C bus mastership
+ * @m: I3C master object
+ * @sec_mst_dyn_addr: address of device
