[PATCH 0/3] i2c: atr: A few i2c-atr fixes

[PATCH 0/3] i2c: atr: A few i2c-atr fixes

[Tomi Valkeinen]

The first two are perhaps not strictly fixes, as they essentially add
support for nested ATRs. The third one is a fix, thus stable is in cc.

 Tomi

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Cosmin Tanislav (1):
      i2c: atr: Allow unmapped addresses from nested ATRs

Tomi Valkeinen (2):
      i2c: atr: Fix lockdep for nested ATRs
      i2c: atr: Fix client detach

 drivers/i2c/i2c-atr.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)
---
base-commit: adc218676eef25575469234709c2d87185ca223a
change-id: 20241121-i2c-atr-fixes-6d52b9f5f0c1

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

[PATCH 1/3] i2c: atr: Allow unmapped addresses from nested ATRs

[Tomi Valkeinen]

From: Cosmin Tanislav <demonsingur@gmail.com>

i2c-atr translates the i2c transactions and forwards them to its parent
i2c bus. Any transaction to an i2c address that has not been mapped on
the i2c-atr will be rejected with an error.

However, if the parent i2c bus is another i2c-atr, the parent i2c-atr
gets a transaction to an i2c address that is not mapped in the parent
i2c-atr, and thus fails.

Relax the checks, and allow non-mapped transactions to fix this issue.

Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/i2c/i2c-atr.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index f21475ae5921..ef5adafa9bf4 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -134,7 +134,6 @@ i2c_atr_find_mapping_by_addr(const struct list_head *list, u16 phys_addr)
 static int i2c_atr_map_msgs(struct i2c_atr_chan *chan, struct i2c_msg *msgs,
 			    int num)
 {
-	struct i2c_atr *atr = chan->atr;
 	static struct i2c_atr_alias_pair *c2a;
 	int i;
 
@@ -157,15 +156,8 @@ static int i2c_atr_map_msgs(struct i2c_atr_chan *chan, struct i2c_msg *msgs,
 
 		c2a = i2c_atr_find_mapping_by_addr(&chan->alias_list,
 						   msgs[i].addr);
-		if (!c2a) {
-			dev_err(atr->dev, "client 0x%02x not mapped!\n",
-				msgs[i].addr);
-
-			while (i--)
-				msgs[i].addr = chan->orig_addrs[i];
-
-			return -ENXIO;
-		}
+		if (!c2a)
+			continue;
 
 		msgs[i].addr = c2a->alias;
 	}

-- 
2.43.0

[PATCH 2/3] i2c: atr: Fix lockdep for nested ATRs

[Tomi Valkeinen]

From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

When we have an ATR, and another ATR as a subdevice of the first ATR,
we get lockdep warnings for the i2c_atr.lock and
i2c_atr_chan.orig_addrs_lock. This is because lockdep uses a static key
for the locks, and doesn't see the locks of the separate ATR instances
as separate.

Fix this by generating a dynamic lock key per lock instance.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/i2c/i2c-atr.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index ef5adafa9bf4..9bdbd94b5054 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -54,6 +54,7 @@ struct i2c_atr_chan {
 
 	/* Lock orig_addrs during xfer */
 	struct mutex orig_addrs_lock;
+	struct lock_class_key orig_addrs_lock_key;
 	u16 *orig_addrs;
 	unsigned int orig_addrs_size;
 };
@@ -84,6 +85,7 @@ struct i2c_atr {
 	struct i2c_algorithm algo;
 	/* lock for the I2C bus segment (see struct i2c_lock_operations) */
 	struct mutex lock;
+	struct lock_class_key lock_key;
 	int max_adapters;
 
 	size_t num_aliases;
@@ -505,7 +507,10 @@ struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
 	if (!atr)
 		return ERR_PTR(-ENOMEM);
 
+	lockdep_register_key(&atr->lock_key);
 	mutex_init(&atr->lock);
+	lockdep_set_class(&atr->lock, &atr->lock_key);
+
 	spin_lock_init(&atr->alias_mask_lock);
 
 	atr->parent = parent;
@@ -535,6 +540,7 @@ struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
 	kfree(atr->aliases);
 err_destroy_mutex:
 	mutex_destroy(&atr->lock);
+	lockdep_unregister_key(&atr->lock_key);
 	kfree(atr);
 
 	return ERR_PTR(ret);
@@ -552,6 +558,7 @@ void i2c_atr_delete(struct i2c_atr *atr)
 	bitmap_free(atr->alias_use_mask);
 	kfree(atr->aliases);
 	mutex_destroy(&atr->lock);
+	lockdep_unregister_key(&atr->lock_key);
 	kfree(atr);
 }
 EXPORT_SYMBOL_NS_GPL(i2c_atr_delete, I2C_ATR);
@@ -586,7 +593,9 @@ int i2c_atr_add_adapter(struct i2c_atr *atr, u32 chan_id,
 	chan->atr = atr;
 	chan->chan_id = chan_id;
 	INIT_LIST_HEAD(&chan->alias_list);
+	lockdep_register_key(&chan->orig_addrs_lock_key);
 	mutex_init(&chan->orig_addrs_lock);
+	lockdep_set_class(&chan->orig_addrs_lock, &chan->orig_addrs_lock_key);
 
 	snprintf(chan->adap.name, sizeof(chan->adap.name), "i2c-%d-atr-%d",
 		 i2c_adapter_id(parent), chan_id);
@@ -646,6 +655,7 @@ int i2c_atr_add_adapter(struct i2c_atr *atr, u32 chan_id,
 err_fwnode_put:
 	fwnode_handle_put(dev_fwnode(&chan->adap.dev));
 	mutex_destroy(&chan->orig_addrs_lock);
+	lockdep_unregister_key(&chan->orig_addrs_lock_key);
 	kfree(chan);
 	return ret;
 }
@@ -679,6 +689,7 @@ void i2c_atr_del_adapter(struct i2c_atr *atr, u32 chan_id)
 
 	fwnode_handle_put(fwnode);
 	mutex_destroy(&chan->orig_addrs_lock);
+	lockdep_unregister_key(&chan->orig_addrs_lock_key);
 	kfree(chan->orig_addrs);
 	kfree(chan);
 }

-- 
2.43.0

Re: [PATCH 2/3] i2c: atr: Fix lockdep for nested ATRs

[Andy Shevchenko]

On Fri, Nov 22, 2024 at 09:51:39AM +0200, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> When we have an ATR, and another ATR as a subdevice of the first ATR,
> we get lockdep warnings for the i2c_atr.lock and
> i2c_atr_chan.orig_addrs_lock. This is because lockdep uses a static key
> for the locks, and doesn't see the locks of the separate ATR instances
> as separate.

...

> +	lockdep_register_key(&atr->lock_key);
>  	mutex_init(&atr->lock);
> +	lockdep_set_class(&atr->lock, &atr->lock_key);

mutext_init_with_key()

...

> +	lockdep_register_key(&chan->orig_addrs_lock_key);
>  	mutex_init(&chan->orig_addrs_lock);
> +	lockdep_set_class(&chan->orig_addrs_lock, &chan->orig_addrs_lock_key);

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] i2c: atr: Fix lockdep for nested ATRs

[kernel test robot]

Hi Tomi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on adc218676eef25575469234709c2d87185ca223a]

url:    https://github.com/intel-lab-lkp/linux/commits/Tomi-Valkeinen/i2c-atr-Allow-unmapped-addresses-from-nested-ATRs/20241125-104419
base:   adc218676eef25575469234709c2d87185ca223a
patch link:    https://lore.kernel.org/r/20241122-i2c-atr-fixes-v1-2-62c51ce790be%40ideasonboard.com
patch subject: [PATCH 2/3] i2c: atr: Fix lockdep for nested ATRs
config: i386-buildonly-randconfig-001-20241125 (https://download.01.org/0day-ci/archive/20241125/202411252015.N7sBYvri-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241125/202411252015.N7sBYvri-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411252015.N7sBYvri-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/i2c/i2c-atr.c:60: warning: Function parameter or struct member 'orig_addrs_lock_key' not described in 'i2c_atr_chan'
>> drivers/i2c/i2c-atr.c:100: warning: Function parameter or struct member 'lock_key' not described in 'i2c_atr'


vim +60 drivers/i2c/i2c-atr.c

a076a860acae77 Luca Ceresoli  2023-06-19   36  
a076a860acae77 Luca Ceresoli  2023-06-19   37  /**
a076a860acae77 Luca Ceresoli  2023-06-19   38   * struct i2c_atr_chan - Data for a channel.
a076a860acae77 Luca Ceresoli  2023-06-19   39   * @adap:            The &struct i2c_adapter for the channel
a076a860acae77 Luca Ceresoli  2023-06-19   40   * @atr:             The parent I2C ATR
a076a860acae77 Luca Ceresoli  2023-06-19   41   * @chan_id:         The ID of this channel
a076a860acae77 Luca Ceresoli  2023-06-19   42   * @alias_list:      List of @struct i2c_atr_alias_pair containing the
a076a860acae77 Luca Ceresoli  2023-06-19   43   *                   assigned aliases
a076a860acae77 Luca Ceresoli  2023-06-19   44   * @orig_addrs_lock: Mutex protecting @orig_addrs
a076a860acae77 Luca Ceresoli  2023-06-19   45   * @orig_addrs:      Buffer used to store the original addresses during transmit
a076a860acae77 Luca Ceresoli  2023-06-19   46   * @orig_addrs_size: Size of @orig_addrs
a076a860acae77 Luca Ceresoli  2023-06-19   47   */
a076a860acae77 Luca Ceresoli  2023-06-19   48  struct i2c_atr_chan {
a076a860acae77 Luca Ceresoli  2023-06-19   49  	struct i2c_adapter adap;
a076a860acae77 Luca Ceresoli  2023-06-19   50  	struct i2c_atr *atr;
a076a860acae77 Luca Ceresoli  2023-06-19   51  	u32 chan_id;
a076a860acae77 Luca Ceresoli  2023-06-19   52  
a076a860acae77 Luca Ceresoli  2023-06-19   53  	struct list_head alias_list;
a076a860acae77 Luca Ceresoli  2023-06-19   54  
a076a860acae77 Luca Ceresoli  2023-06-19   55  	/* Lock orig_addrs during xfer */
a076a860acae77 Luca Ceresoli  2023-06-19   56  	struct mutex orig_addrs_lock;
f2d3b6b436282b Tomi Valkeinen 2024-11-22   57  	struct lock_class_key orig_addrs_lock_key;
a076a860acae77 Luca Ceresoli  2023-06-19   58  	u16 *orig_addrs;
a076a860acae77 Luca Ceresoli  2023-06-19   59  	unsigned int orig_addrs_size;
a076a860acae77 Luca Ceresoli  2023-06-19  @60  };
a076a860acae77 Luca Ceresoli  2023-06-19   61  
a076a860acae77 Luca Ceresoli  2023-06-19   62  /**
a076a860acae77 Luca Ceresoli  2023-06-19   63   * struct i2c_atr - The I2C ATR instance
a076a860acae77 Luca Ceresoli  2023-06-19   64   * @parent:    The parent &struct i2c_adapter
a076a860acae77 Luca Ceresoli  2023-06-19   65   * @dev:       The device that owns the I2C ATR instance
a076a860acae77 Luca Ceresoli  2023-06-19   66   * @ops:       &struct i2c_atr_ops
a076a860acae77 Luca Ceresoli  2023-06-19   67   * @priv:      Private driver data, set with i2c_atr_set_driver_data()
a076a860acae77 Luca Ceresoli  2023-06-19   68   * @algo:      The &struct i2c_algorithm for adapters
a076a860acae77 Luca Ceresoli  2023-06-19   69   * @lock:      Lock for the I2C bus segment (see &struct i2c_lock_operations)
a076a860acae77 Luca Ceresoli  2023-06-19   70   * @max_adapters: Maximum number of adapters this I2C ATR can have
a076a860acae77 Luca Ceresoli  2023-06-19   71   * @num_aliases: Number of aliases in the aliases array
a076a860acae77 Luca Ceresoli  2023-06-19   72   * @aliases:   The aliases array
a076a860acae77 Luca Ceresoli  2023-06-19   73   * @alias_mask_lock: Lock protecting alias_use_mask
a076a860acae77 Luca Ceresoli  2023-06-19   74   * @alias_use_mask: Bitmask for used aliases in aliases array
a076a860acae77 Luca Ceresoli  2023-06-19   75   * @i2c_nb:    Notifier for remote client add & del events
a076a860acae77 Luca Ceresoli  2023-06-19   76   * @adapter:   Array of adapters
a076a860acae77 Luca Ceresoli  2023-06-19   77   */
a076a860acae77 Luca Ceresoli  2023-06-19   78  struct i2c_atr {
a076a860acae77 Luca Ceresoli  2023-06-19   79  	struct i2c_adapter *parent;
a076a860acae77 Luca Ceresoli  2023-06-19   80  	struct device *dev;
a076a860acae77 Luca Ceresoli  2023-06-19   81  	const struct i2c_atr_ops *ops;
a076a860acae77 Luca Ceresoli  2023-06-19   82  
a076a860acae77 Luca Ceresoli  2023-06-19   83  	void *priv;
a076a860acae77 Luca Ceresoli  2023-06-19   84  
a076a860acae77 Luca Ceresoli  2023-06-19   85  	struct i2c_algorithm algo;
a076a860acae77 Luca Ceresoli  2023-06-19   86  	/* lock for the I2C bus segment (see struct i2c_lock_operations) */
a076a860acae77 Luca Ceresoli  2023-06-19   87  	struct mutex lock;
f2d3b6b436282b Tomi Valkeinen 2024-11-22   88  	struct lock_class_key lock_key;
a076a860acae77 Luca Ceresoli  2023-06-19   89  	int max_adapters;
a076a860acae77 Luca Ceresoli  2023-06-19   90  
a076a860acae77 Luca Ceresoli  2023-06-19   91  	size_t num_aliases;
a076a860acae77 Luca Ceresoli  2023-06-19   92  	const u16 *aliases;
a076a860acae77 Luca Ceresoli  2023-06-19   93  	/* Protects alias_use_mask */
a076a860acae77 Luca Ceresoli  2023-06-19   94  	spinlock_t alias_mask_lock;
a076a860acae77 Luca Ceresoli  2023-06-19   95  	unsigned long *alias_use_mask;
a076a860acae77 Luca Ceresoli  2023-06-19   96  
a076a860acae77 Luca Ceresoli  2023-06-19   97  	struct notifier_block i2c_nb;
a076a860acae77 Luca Ceresoli  2023-06-19   98  
3a133a4e44554b Kees Cook      2023-09-22   99  	struct i2c_adapter *adapter[] __counted_by(max_adapters);
a076a860acae77 Luca Ceresoli  2023-06-19 @100  };
a076a860acae77 Luca Ceresoli  2023-06-19  101  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 3/3] i2c: atr: Fix client detach

[Tomi Valkeinen]

From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

i2c-atr catches the BUS_NOTIFY_DEL_DEVICE event on the bus and removes
the translation by calling i2c_atr_detach_client().

However, BUS_NOTIFY_DEL_DEVICE happens when the device is about to be
removed from this bus, i.e. before removal, and thus before calling
.remove() on the driver. If the driver happens to do any i2c
transactions in its remove(), they will fail.

Fix this by catching BUS_NOTIFY_REMOVED_DEVICE instead, thus removing
the translation only after the device is actually removed.

Fixes: a076a860acae ("media: i2c: add I2C Address Translator (ATR) support")
Cc: stable@vger.kernel.org
Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/i2c/i2c-atr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index 9bdbd94b5054..2ce12d42c24f 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -406,7 +406,7 @@ static int i2c_atr_bus_notifier_call(struct notifier_block *nb,
 				dev_name(dev), ret);
 		break;
 
-	case BUS_NOTIFY_DEL_DEVICE:
+	case BUS_NOTIFY_REMOVED_DEVICE:
 		i2c_atr_detach_client(client->adapter, client);
 		break;
 

-- 
2.43.0

Re: [PATCH 3/3] i2c: atr: Fix client detach

[Andy Shevchenko]

On Fri, Nov 22, 2024 at 09:51:40AM +0200, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> i2c-atr catches the BUS_NOTIFY_DEL_DEVICE event on the bus and removes
> the translation by calling i2c_atr_detach_client().
> 
> However, BUS_NOTIFY_DEL_DEVICE happens when the device is about to be
> removed from this bus, i.e. before removal, and thus before calling
> .remove() on the driver. If the driver happens to do any i2c
> transactions in its remove(), they will fail.
> 
> Fix this by catching BUS_NOTIFY_REMOVED_DEVICE instead, thus removing
> the translation only after the device is actually removed.

...

> Fixes: a076a860acae ("media: i2c: add I2C Address Translator (ATR) support")

Fixes should lead the patch series, and other way around.

-- 
With Best Regards,
Andy Shevchenko