From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Thu, Aug 29, 2019 at 16:37:09

On Thu, 29 Aug 2019 15:23:30 +0000
Vitor Soares [off-list ref] wrote:

quoted

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Thu, Aug 29, 2019 at 15:35:20

quoted

On Thu, 29 Aug 2019 13:53:24 +0000
Vitor Soares [off-list ref] wrote:
  

quoted

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Thu, Aug 29, 2019 at 11:41:15
  

quoted

+Przemek

Please try to Cc active I3C contributors so they get a chance to
comment on your patches.    

I can do that next time.
  

quoted

On Thu, 29 Aug 2019 12:19:32 +0200
Vitor Soares [off-list ref] wrote:
    

quoted

On pre_assing_dyn_addr() the devices that fail:
  i3c_master_setdasa_locked()
  i3c_master_reattach_i3c_dev()
  i3c_master_retrieve_dev_info()

are kept in memory and master->bus.devs list. This makes the i3c devices
without a dynamic address are sent on DEFSLVS CCC command. Fix this by
detaching and freeing the devices that fail on pre_assign_dyn_addr().    

I don't think removing those entries is a good strategy, as one might
want to try to use a different dynamic address if the requested one
is not available.    

Do you mean same 'assigned-address' attribute in DT?  

Yes, or say it's another device that got the address we want and this
device doesn't want to release the address (I'm assuming the !SA case).
  

quoted

If so, it is checked here:

static int i3c_master_bus_init(struct i3c_master_controller *master)
...
	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
		struct i3c_device_info info = {
			.static_addr = i3cboardinfo->static_addr,
		};

		if (i3cboardinfo->init_dyn_addr) {
			status = i3c_bus_get_addr_slot_status(&master->bus,
			^
						i3cboardinfo->init_dyn_addr);
			if (status != I3C_ADDR_SLOT_FREE) {
				ret = -EBUSY;
				goto err_detach_devs;
			}
		}

		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
		if (IS_ERR(i3cdev)) {
			ret = PTR_ERR(i3cdev);
			goto err_detach_devs;
		}

		i3cdev->boardinfo = i3cboardinfo;

		ret = i3c_master_attach_i3c_dev(master, i3cdev);
		if (ret) {
			i3c_master_free_i3c_dev(i3cdev);
			goto err_detach_devs;
		}
	}
...

and later if it fails i3c_master_pre_assign_dyn_addr(), the device can 
participate in Enter Dynamic Address Assignment process.
I may need to check the boardinfo->init_dyn_addr status on 
i3c_master_add_i3c_dev_locked before i3c_master_setnewda_locked().  

I need to double check but I thought we were already handling that case
properly.  

Yes, it is handled in the code above.

No, I meant the 'assign init_dyn_addr even if !SA', and the code I
pointed in my other reply tends to confirm that this is something we
already take into account (maybe not correctly, but the code is here).

Please check my last comment in patch 2/4.

quoted

quoted

quoted

quoted

Why not simply skipping entries that have ->dyn_addr
set to 0 when preparing a DEFSLVS frame    

I considered that solution too but if the device isn't enumerated why 
should it be attached and kept in memory?  

Might be a device that supports HJ, and in that case we might want the
controller to reserve a slot in its device table for that device.
Anyway, it doesn't hurt to have it around as long as we don't pass the
device through DEFSLVS if it doesn't have a dynamic address. I really
prefer to keep the logic unchanged and fix it if it needs to be fixed.  

Well, we aren't reserving a slot because we need another one to attach 
the device when it is enumerated and hence a device may be using 2 slots 
in the controller.

Right, you shouldn't reserve a slot when ->static_address == 0 &&
->dynamic_address == 0, but I still don't see where the problem is with
the solution we have right now, sorry. Note that even if you reserve a
slot in that case, the device only occupies 2 slots for a short amount
of time, because the add_i3c_dev() logic will detect that the descriptor
already exists and reattach the device with its new address.

I understand that but if we have limited resources it is a problem.

quoted

It may cause problems in HC with reduced slots and it is another reason 
why I think we should detach device without dynamic address after the 
enumeration phase.

Can you please try the approach I suggest? => fix the existing logic to
make it work without this "free undiscovered dev desc, reallocate later"
dance.

I can do that but we still have the issue above to solve and figure how 
to set a dynamic address for devices without static address.