Re: [PATCH v7 16/24] i2c: allow adapter drivers to override the adapter locking

[Wolfram Sang]

> A question on best practices here... I already did a v8 (but only as
> a branch) so I think this will be v9, bit that's a minor detail. The
> real question is what I should do about patches 1-15 that are already
> in next? Send them too? If not, should I send 16-24 with the same old
> patch numbers or should they be numbered 1-9 now? And should such a
> shortened series be rebased on 1-15 in next?
> 
> Or does it not really matter?

Easiest for me is:

Send as v9, only the patches not yet applied, numbered from 1-9, based
on my for-next.



signature.asc
Description: PGP signature

Re: [PATCH v7 16/24] i2c: allow adapter drivers to override the adapter locking

[Wolfram Sang]

Hi Peter,

thanks for the detailed explanation!

> So maybe there should be only one flag, e.g. I2C_LOCK_ROOT_ADAPTER?
> I.e. perhaps leave the future for later?

I think this makes the current code easier understandable at this point,
but I'll leave the decision to you. I am fine with both. Maybe a few
words of explanation would be good if you want to keep both flags.

> Hmmm, I just now realized that you were not really suggesting any
> changes other than to the commit message. Oh well, I can perhaps
> rephrase some of the above in the commit message if you think that
> we should not unnecessarily touch the code at this point...

Yes, updated commit description is enough for me now. If you want to
change to one flag, we should do it incrementally. I think I can apply
this as a fixup until around rc3 I'd say.

> > I think this kerneldoc should be moved to i2c_lock_adapter and/or
> > i2c_lock_bus() which are now in i2c.h. This is what users will use, not
> > this static, adapter-specific implementation. I think it is enough to
> > have a comment here explaining what is special in handling adapters.
> 
> Yes, I was not really satisfied with having documentation on static
> functions. But if I move it, there is no natural home for the current
> i2c_trylock_adapter docs, and I'd hate killing documentation that
> still applies. Do you have a suggestion? Maybe keep that one doc at
> the static i2c_trylock_adapter for now and move it to ->trylock_bus
> when someone decides to write kerneldoc for struct i2c_adapter?

Well, because I think redundancy is acceptable when it comes to
documentation, how about keeping the chunks you already have and copy an
adapted one over to the functions in i2c.h?

Regards,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCH v7 16/24] i2c: allow adapter drivers to override the adapter locking

[Wolfram Sang]

On Wed, Apr 20, 2016 at 05:17:56PM +0200, Peter Rosin wrote:
> Add i2c_lock_bus() and i2c_unlock_bus(), which call the new lock_bus and
> unlock_bus ops in the adapter. These funcs/ops take an additional flags
> argument that indicates for what purpose the adapter is locked.
> 
> There are two flags, I2C_LOCK_ADAPTER and I2C_LOCK_SEGMENT, but they are
> both implemented the same. For now. Locking the adapter means that the
> whole bus is locked, locking the segment means that only the current bus
> segment is locked (i.e. i2c traffic on the parent side of mux is still
> allowed even if the child side of the mux is locked.
> 
> Also support a trylock_bus op (but no function to call it, as it is not
> expected to be needed outside of the i2c core).
> 
> Implement i2c_lock_adapter/i2c_unlock_adapter in terms of the new locking
> scheme (i.e. lock with the I2C_LOCK_ADAPTER flag).
> 
> Annotate some of the locking with explicit I2C_LOCK_SEGMENT flags.

Can you explain a little why it is SEGMENT and not ADAPTER here? That
probably makes it easier to get into this patch.

And to double check my understanding: I was surprised to not see any
i2c_lock_adapter() or I2C_LOCK_ADAPTER in action. This is because muxes
call I2C_LOCK_SEGMENT on their parent which in case of the parent being
the root adapter is essentially the same as I2C_LOCK_ADAPTER. Correct?

> 
> Signed-off-by: Peter Rosin 
> ---
>  drivers/i2c/i2c-core.c | 46 --
>  include/linux/i2c.h| 28 ++--
>  2 files changed, 54 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 0f2f8484e8ec..21f46d011c33 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -960,10 +960,12 @@ static int i2c_check_addr_busy(struct i2c_adapter 
> *adapter, int addr)
>  }
>  
>  /**
> - * i2c_lock_adapter - Get exclusive access to an I2C bus segment
> + * i2c_adapter_lock_bus - Get exclusive access to an I2C bus segment
>   * @adapter: Target I2C bus segment
> + * @flags: I2C_LOCK_ADAPTER locks the root i2c adapter, I2C_LOCK_SEGMENT
> + *   locks only this branch in the adapter tree
>   */

I think this kerneldoc should be moved to i2c_lock_adapter and/or
i2c_lock_bus() which are now in i2c.h. This is what users will use, not
this static, adapter-specific implementation. I think it is enough to
have a comment here explaining what is special in handling adapters.

Thanks,

   Wolfram
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 16/24] i2c: allow adapter drivers to override the adapter locking

[Antti Palosaari]

On 04/29/2016 12:16 PM, Peter Rosin wrote:

On 2016-04-29 09:16, Wolfram Sang wrote:

Yes, obviously... I'll make that change locally and wait for the rest.

Another nit: You could use '--strict' with checkpatch and see if you
want to fix the issues reported. I am not keen on those (except for
'space around operators'), it's a matter of taste I guess, but maybe you
like some of the suggestions.


Yes, they look like reasonable complaints.

So, I fixed all of them locally except the complaint about lack of comment
on the new struct mutex member in struct si2168_dev (patch 21/24),
because that patch is Anttis and he's the maintainer of that driver...

Antti, if you want that fixed as part of this series, send a suitable comment
for the mutex this way and I'll incorporate it.


Ah, I never ran checkpatch with --strict option...

CHECK: struct mutex definition without comment
#202: FILE: drivers/media/dvb-frontends/si2168_priv.h:32:
+   struct mutex i2c_mutex;

If you wish you could add some comment for it, but for me it is still 
pretty much self explaining. It is lock to protect firmware command 
execution. Command is executed always with I2C write and then poll reply 
using I2C read until it timeouts or answers with "ready" status.


regards
Antti

--
http://palosaari.fi/
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 16/24] i2c: allow adapter drivers to override the adapter locking

[Peter Rosin]

On 2016-04-29 09:16, Wolfram Sang wrote:
>> Yes, obviously... I'll make that change locally and wait for the rest.
> Another nit: You could use '--strict' with checkpatch and see if you
> want to fix the issues reported. I am not keen on those (except for
> 'space around operators'), it's a matter of taste I guess, but maybe you
> like some of the suggestions.
>
Yes, they look like reasonable complaints.

So, I fixed all of them locally except the complaint about lack of comment
on the new struct mutex member in struct si2168_dev (patch 21/24),
because that patch is Anttis and he's the maintainer of that driver...

Antti, if you want that fixed as part of this series, send a suitable comment
for the mutex this way and I'll incorporate it.

Cheers,
Peter

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 16/24] i2c: allow adapter drivers to override the adapter locking

[Peter Rosin]

On 2016-04-28 22:50, Wolfram Sang wrote:
> On Wed, Apr 20, 2016 at 05:17:56PM +0200, Peter Rosin wrote:
>> Add i2c_lock_bus() and i2c_unlock_bus(), which call the new lock_bus and
>> unlock_bus ops in the adapter. These funcs/ops take an additional flags
>> argument that indicates for what purpose the adapter is locked.
>>
>> There are two flags, I2C_LOCK_ADAPTER and I2C_LOCK_SEGMENT, but they are
>> both implemented the same. For now. Locking the adapter means that the
>> whole bus is locked, locking the segment means that only the current bus
>> segment is locked (i.e. i2c traffic on the parent side of mux is still
>> allowed even if the child side of the mux is locked.
>>
>> Also support a trylock_bus op (but no function to call it, as it is not
>> expected to be needed outside of the i2c core).
>>
>> Implement i2c_lock_adapter/i2c_unlock_adapter in terms of the new locking
>> scheme (i.e. lock with the I2C_LOCK_ADAPTER flag).
>>
>> Annotate some of the locking with explicit I2C_LOCK_SEGMENT flags.
>>
>> Signed-off-by: Peter Rosin 
> Letting you know that I start reviewing the 2nd part of your series. Did
> the first glimpse today. Will hopefully do the in-depth part this
> weekend. One thing already:
>
>> +static void i2c_adapter_lock_bus(struct i2c_adapter *adapter, int flags)
> Shouldn't flags be unsigned?
>

Yes, obviously... I'll make that change locally and wait for the rest.

Cheers,
Peter

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 16/24] i2c: allow adapter drivers to override the adapter locking

[Peter Rosin]

Add i2c_lock_bus() and i2c_unlock_bus(), which call the new lock_bus and
unlock_bus ops in the adapter. These funcs/ops take an additional flags
argument that indicates for what purpose the adapter is locked.

There are two flags, I2C_LOCK_ADAPTER and I2C_LOCK_SEGMENT, but they are
both implemented the same. For now. Locking the adapter means that the
whole bus is locked, locking the segment means that only the current bus
segment is locked (i.e. i2c traffic on the parent side of mux is still
allowed even if the child side of the mux is locked.

Also support a trylock_bus op (but no function to call it, as it is not
expected to be needed outside of the i2c core).

Implement i2c_lock_adapter/i2c_unlock_adapter in terms of the new locking
scheme (i.e. lock with the I2C_LOCK_ADAPTER flag).

Annotate some of the locking with explicit I2C_LOCK_SEGMENT flags.

Signed-off-by: Peter Rosin 
---
 drivers/i2c/i2c-core.c | 46 --
 include/linux/i2c.h| 28 ++--
 2 files changed, 54 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 0f2f8484e8ec..21f46d011c33 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -960,10 +960,12 @@ static int i2c_check_addr_busy(struct i2c_adapter 
*adapter, int addr)
 }
 
 /**
- * i2c_lock_adapter - Get exclusive access to an I2C bus segment
+ * i2c_adapter_lock_bus - Get exclusive access to an I2C bus segment
  * @adapter: Target I2C bus segment
+ * @flags: I2C_LOCK_ADAPTER locks the root i2c adapter, I2C_LOCK_SEGMENT
+ * locks only this branch in the adapter tree
  */
-void i2c_lock_adapter(struct i2c_adapter *adapter)
+static void i2c_adapter_lock_bus(struct i2c_adapter *adapter, int flags)
 {
struct i2c_adapter *parent = i2c_parent_is_i2c_adapter(adapter);
 
@@ -972,27 +974,30 @@ void i2c_lock_adapter(struct i2c_adapter *adapter)
else
rt_mutex_lock(>bus_lock);
 }
-EXPORT_SYMBOL_GPL(i2c_lock_adapter);
 
 /**
- * i2c_trylock_adapter - Try to get exclusive access to an I2C bus segment
+ * i2c_adapter_trylock_bus - Try to get exclusive access to an I2C bus segment
  * @adapter: Target I2C bus segment
+ * @flags: I2C_LOCK_ADAPTER trylocks the root i2c adapter, I2C_LOCK_SEGMENT
+ * trylocks only this branch in the adapter tree
  */
-static int i2c_trylock_adapter(struct i2c_adapter *adapter)
+static int i2c_adapter_trylock_bus(struct i2c_adapter *adapter, int flags)
 {
struct i2c_adapter *parent = i2c_parent_is_i2c_adapter(adapter);
 
if (parent)
-   return i2c_trylock_adapter(parent);
+   return parent->trylock_bus(parent, flags);
else
return rt_mutex_trylock(>bus_lock);
 }
 
 /**
- * i2c_unlock_adapter - Release exclusive access to an I2C bus segment
+ * i2c_adapter_unlock_bus - Release exclusive access to an I2C bus segment
  * @adapter: Target I2C bus segment
+ * @flags: I2C_LOCK_ADAPTER unlocks the root i2c adapter, I2C_LOCK_SEGMENT
+ * unlocks only this branch in the adapter tree
  */
-void i2c_unlock_adapter(struct i2c_adapter *adapter)
+static void i2c_adapter_unlock_bus(struct i2c_adapter *adapter, int flags)
 {
struct i2c_adapter *parent = i2c_parent_is_i2c_adapter(adapter);
 
@@ -1001,7 +1006,6 @@ void i2c_unlock_adapter(struct i2c_adapter *adapter)
else
rt_mutex_unlock(>bus_lock);
 }
-EXPORT_SYMBOL_GPL(i2c_unlock_adapter);
 
 static void i2c_dev_set_name(struct i2c_adapter *adap,
 struct i2c_client *client)
@@ -1547,6 +1551,12 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
return -EINVAL;
}
 
+   if (!adap->lock_bus) {
+   adap->lock_bus = i2c_adapter_lock_bus;
+   adap->trylock_bus = i2c_adapter_trylock_bus;
+   adap->unlock_bus = i2c_adapter_unlock_bus;
+   }
+
rt_mutex_init(>bus_lock);
mutex_init(>userspace_clients_lock);
INIT_LIST_HEAD(>userspace_clients);
@@ -2315,16 +2325,16 @@ int i2c_transfer(struct i2c_adapter *adap, struct 
i2c_msg *msgs, int num)
 #endif
 
if (in_atomic() || irqs_disabled()) {
-   ret = i2c_trylock_adapter(adap);
+   ret = adap->trylock_bus(adap, I2C_LOCK_SEGMENT);
if (!ret)
/* I2C activity is ongoing. */
return -EAGAIN;
} else {
-   i2c_lock_adapter(adap);
+   i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
}
 
ret = __i2c_transfer(adap, msgs, num);
-   i2c_unlock_adapter(adap);
+   i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
 
return ret;
} else {
@@ -3099,7 +3109,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, 
unsigned short flags,
flags &=