[PATCH] i3c: make sure the PID is set before registering the device

[PATCH] i3c: make sure the PID is set before registering the device

[Przemysław Gaj]

From: Przemyslaw Gaj <pgaj@cadence.com>

Provisioned ID (PID) is the value with which device drivers are
matched. I check the value before registering the device.

Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
---
 drivers/i3c/master.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 043691656245..07ea8b0b7736 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1449,7 +1449,8 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
 		return;
 
 	i3c_bus_for_each_i3cdev(&master->bus, desc) {
-		if (desc->dev || !desc->info.dyn_addr || desc == master->this)
+		if (desc->dev || !desc->info.dyn_addr ||
+		    desc == master->this || !desc->info.pid)
 			continue;
 
 		desc->dev = kzalloc(sizeof(*desc->dev), GFP_KERNEL);
-- 
2.18.0


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH] i3c: make sure the PID is set before registering the device

[Boris Brezillon]

On Sun, 8 Dec 2019 15:18:34 +0100
Przemysław Gaj <pgaj@cadence.com> wrote:

> From: Przemyslaw Gaj <pgaj@cadence.com>
> 
> Provisioned ID (PID) is the value with which device drivers are
> matched. I check the value before registering the device.
> 

Can this situation (having a device with a DA but without a valid PID)
happen right now or is this something you need to support secondary
masters who receive device DA (through DEFSVLS) without being able to
query extra information until they own the bus?

> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> ---
>  drivers/i3c/master.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 043691656245..07ea8b0b7736 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1449,7 +1449,8 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>  		return;
>  
>  	i3c_bus_for_each_i3cdev(&master->bus, desc) {
> -		if (desc->dev || !desc->info.dyn_addr || desc == master->this)
> +		if (desc->dev || !desc->info.dyn_addr ||
> +		    desc == master->this || !desc->info.pid)
>  			continue;
>  
>  		desc->dev = kzalloc(sizeof(*desc->dev), GFP_KERNEL);


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH] i3c: make sure the PID is set before registering the device

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Mon, Dec 09, 2019 at 09:47:11

> On Sun, 8 Dec 2019 15:18:34 +0100
> Przemysław Gaj <pgaj@cadence.com> wrote:
> 
> > From: Przemyslaw Gaj <pgaj@cadence.com>
> > 
> > Provisioned ID (PID) is the value with which device drivers are
> > matched. I check the value before registering the device.
> > 
> 
> Can this situation (having a device with a DA but without a valid PID)
> happen right now or is this something you need to support secondary
> masters who receive device DA (through DEFSVLS) without being able to
> query extra information until they own the bus?

This is the use case where a device fails the 
i3c_master_pre_assign_dyn_addr() and I already mention it on [1].
I still think the best way is to detach/free the devices that fails 
during i3c_master_pre_assign_dyn_addr().

> 
> > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> > ---
> >  drivers/i3c/master.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 043691656245..07ea8b0b7736 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -1449,7 +1449,8 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
> >  		return;
> >  
> >  	i3c_bus_for_each_i3cdev(&master->bus, desc) {
> > -		if (desc->dev || !desc->info.dyn_addr || desc == master->this)
> > +		if (desc->dev || !desc->info.dyn_addr ||
> > +		    desc == master->this || !desc->info.pid)
> >  			continue;
> >  
> >  		desc->dev = kzalloc(sizeof(*desc->dev), GFP_KERNEL);

Best regards,
Vitor Soares

[1] https://patchwork.kernel.org/patch/11132601/
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH] i3c: make sure the PID is set before registering the device

[Boris Brezillon]

On Mon, 9 Dec 2019 12:20:03 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> Hi Boris,
> 
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Mon, Dec 09, 2019 at 09:47:11
> 
> > On Sun, 8 Dec 2019 15:18:34 +0100
> > Przemysław Gaj <pgaj@cadence.com> wrote:
> >   
> > > From: Przemyslaw Gaj <pgaj@cadence.com>
> > > 
> > > Provisioned ID (PID) is the value with which device drivers are
> > > matched. I check the value before registering the device.
> > >   
> > 
> > Can this situation (having a device with a DA but without a valid PID)
> > happen right now or is this something you need to support secondary
> > masters who receive device DA (through DEFSVLS) without being able to
> > query extra information until they own the bus?  
> 
> This is the use case where a device fails the 
> i3c_master_pre_assign_dyn_addr() and I already mention it on [1].
> I still think the best way is to detach/free the devices that fails 
> during i3c_master_pre_assign_dyn_addr().

And I disagree (I gave my arguments already, so won't repeat them
here). Can we move on please? Can you send a new version that does what
I suggest, or should I ask Przemek to do it?

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH] i3c: make sure the PID is set before registering the device

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Mon, Dec 09, 2019 at 12:26:16

> On Mon, 9 Dec 2019 12:20:03 +0000
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > Hi Boris,
> > 
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> > Date: Mon, Dec 09, 2019 at 09:47:11
> > 
> > > On Sun, 8 Dec 2019 15:18:34 +0100
> > > Przemysław Gaj <pgaj@cadence.com> wrote:
> > >   
> > > > From: Przemyslaw Gaj <pgaj@cadence.com>
> > > > 
> > > > Provisioned ID (PID) is the value with which device drivers are
> > > > matched. I check the value before registering the device.
> > > >   
> > > 
> > > Can this situation (having a device with a DA but without a valid PID)
> > > happen right now or is this something you need to support secondary
> > > masters who receive device DA (through DEFSVLS) without being able to
> > > query extra information until they own the bus?  
> > 
> > This is the use case where a device fails the 
> > i3c_master_pre_assign_dyn_addr() and I already mention it on [1].
> > I still think the best way is to detach/free the devices that fails 
> > during i3c_master_pre_assign_dyn_addr().
> 
> And I disagree (I gave my arguments already, so won't repeat them
> here).

Sorry Boris, did you give? I ask you several times why to keep non DA 
devices attached to the bus, yet you wasn't able to give me a technical 
reason. Even from device model you should kept them.
Honestly, I'm starting ask myself if this is something against because 
every time that I'm trying to improve something I just see difficulty 
from your side.

> Can we move on please?

I think you should start learning to listen the other and have the 
greatness to accept the others experience and opinions. With your 
experience you should already learn that.
The previous patch makes all the sense yet your response was - I am not 
convinced. Could you please give a proper justification? At least test 
what I spend time to do?

> Can you send a new version that does what
> I suggest, or should I ask Przemek to do it?

Does the ethics approve such attitude? Don't you see with kind of 
attitude you are just putting people away of this subsystem?

> 
> _______________________________________________
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Di3c&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=cXyPCHO2vhcyD3aN-LnqOqzodbB9g-utJXeWOQRkJmk&s=odrtYhb-IaK-dfTzi1p7TwH2YJcnZ3RSdNNrKJjXFdM&e= 

Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH] i3c: make sure the PID is set before registering the device

[Przemysław Gaj]

Hi Vitor,

The 12/10/2019 14:28, Vitor Soares wrote:
> 
> Hi Boris,
> 
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Mon, Dec 09, 2019 at 12:26:16
> 
> > On Mon, 9 Dec 2019 12:20:03 +0000
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > 
> > > Hi Boris,
> > > 
> > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > Date: Mon, Dec 09, 2019 at 09:47:11
> > > 
> > > > On Sun, 8 Dec 2019 15:18:34 +0100
> > > > Przemysław Gaj <pgaj@cadence.com> wrote:
> > > >   
> > > > > From: Przemyslaw Gaj <pgaj@cadence.com>
> > > > > 
> > > > > Provisioned ID (PID) is the value with which device drivers are
> > > > > matched. I check the value before registering the device.
> > > > >   
> > > > 
> > > > Can this situation (having a device with a DA but without a valid PID)
> > > > happen right now or is this something you need to support secondary
> > > > masters who receive device DA (through DEFSVLS) without being able to
> > > > query extra information until they own the bus?  
> > > 
> > > This is the use case where a device fails the 
> > > i3c_master_pre_assign_dyn_addr() and I already mention it on [1].
> > > I still think the best way is to detach/free the devices that fails 
> > > during i3c_master_pre_assign_dyn_addr().
> > 
> > And I disagree (I gave my arguments already, so won't repeat them
> > here).
> 
> Sorry Boris, did you give? I ask you several times why to keep non DA 
> devices attached to the bus, yet you wasn't able to give me a technical 
> reason. Even from device model you should kept them.

I think we should keep them because framework should still have the
information about all the devices. We already discussed that, right?
Especially, when we have to deal with group addresses soon, it's better
to keep them.

> Honestly, I'm starting ask myself if this is something against because 
> every time that I'm trying to improve something I just see difficulty 
> from your side.

Don't forget that my patches are accepted after 6th, 7th versions. I
think that it just should work like that. This makes things better :)

> 
> > Can we move on please?
> 
> I think you should start learning to listen the other and have the 
> greatness to accept the others experience and opinions. With your 
> experience you should already learn that.
> The previous patch makes all the sense yet your response was - I am not 
> convinced. Could you please give a proper justification? At least test 
> what I spend time to do?
> 
> > Can you send a new version that does what
> > I suggest, or should I ask Przemek to do it?
> 
> Does the ethics approve such attitude? Don't you see with kind of 
> attitude you are just putting people away of this subsystem?
> 
> > 
> > _______________________________________________
> > linux-i3c mailing list
> > linux-i3c@lists.infradead.org
> > https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Di3c&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=cXyPCHO2vhcyD3aN-LnqOqzodbB9g-utJXeWOQRkJmk&s=odrtYhb-IaK-dfTzi1p7TwH2YJcnZ3RSdNNrKJjXFdM&e= 
> 
> Best regards,
> Vitor Soares
> 

-- 
-- 
Regards,
Przemek

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH] i3c: make sure the PID is set before registering the device

[Vitor Soares]

Hi Przemysław,

From: Przemysław Gaj <pgaj@cadence.com>
Date: Tue, Dec 10, 2019 at 14:42:33

> Hi Vitor,
> 
> The 12/10/2019 14:28, Vitor Soares wrote:
> > 
> > Hi Boris,
> > 
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> > Date: Mon, Dec 09, 2019 at 12:26:16
> > 
> > > On Mon, 9 Dec 2019 12:20:03 +0000
> > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > 
> > > > Hi Boris,
> > > > 
> > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > Date: Mon, Dec 09, 2019 at 09:47:11
> > > > 
> > > > > On Sun, 8 Dec 2019 15:18:34 +0100
> > > > > Przemysław Gaj <pgaj@cadence.com> wrote:
> > > > >   
> > > > > > From: Przemyslaw Gaj <pgaj@cadence.com>
> > > > > > 
> > > > > > Provisioned ID (PID) is the value with which device drivers are
> > > > > > matched. I check the value before registering the device.
> > > > > >   
> > > > > 
> > > > > Can this situation (having a device with a DA but without a valid PID)
> > > > > happen right now or is this something you need to support secondary
> > > > > masters who receive device DA (through DEFSVLS) without being able to
> > > > > query extra information until they own the bus?  
> > > > 
> > > > This is the use case where a device fails the 
> > > > i3c_master_pre_assign_dyn_addr() and I already mention it on [1].
> > > > I still think the best way is to detach/free the devices that fails 
> > > > during i3c_master_pre_assign_dyn_addr().
> > > 
> > > And I disagree (I gave my arguments already, so won't repeat them
> > > here).
> > 
> > Sorry Boris, did you give? I ask you several times why to keep non DA 
> > devices attached to the bus, yet you wasn't able to give me a technical 
> > reason. Even from device model you should kept them.
> 
> I think we should keep them because framework should still have the
> information about all the devices. We already discussed that, right?
> Especially, when we have to deal with group addresses soon, it's better
> to keep them.

Could you please point me where do we need it? We still have the 
boardinfo that hold DT information which is what we need.

If a device doesn't have DA, why do we need it? By definition the i3c dev 
desc is the discovered devices, if it doesn't have an DA isn't discovered 
yet.

> 
> > Honestly, I'm starting ask myself if this is something against because 
> > every time that I'm trying to improve something I just see difficulty 
> > from your side.
> 
> Don't forget that my patches are accepted after 6th, 7th versions. I
> think that it just should work like that. This makes things better :)

I don't have issue in send multiple versions of my patches as long as 
they are improved. In this case,  I don't agree with Boris's requirements 
because in my tests I still have issues.
Please make the exercise and check what needs to be done when a device 
fails in i3c_master_pre_assign_dyn_addr() so everything goes ok and what 
it is the advantage of kept i3c_dev_desc from those devices.

And still I have issue on my side. 

Did you test the previous patch? Did you find any issue? If so, you 
wouldn't need the fix the i3c_master_register_new_i3c_devs() which is the 
last instance of registering an i3c device.
I would ask you to verify in v1.1 spec the CCCs table and verify if fix 
the i3c_master_register_new_i3c_devs() is correct according what is 
there.

> 
> > 
> > > Can we move on please?
> > 
> > I think you should start learning to listen the other and have the 
> > greatness to accept the others experience and opinions. With your 
> > experience you should already learn that.
> > The previous patch makes all the sense yet your response was - I am not 
> > convinced. Could you please give a proper justification? At least test 
> > what I spend time to do?
> > 
> > > Can you send a new version that does what
> > > I suggest, or should I ask Przemek to do it?
> > 
> > Does the ethics approve such attitude? Don't you see with kind of 
> > attitude you are just putting people away of this subsystem?
> > 
> > > 
> > > _______________________________________________
> > > linux-i3c mailing list
> > > linux-i3c@lists.infradead.org
> > > https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Di3c&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=cXyPCHO2vhcyD3aN-LnqOqzodbB9g-utJXeWOQRkJmk&s=odrtYhb-IaK-dfTzi1p7TwH2YJcnZ3RSdNNrKJjXFdM&e= 
> > 
> > Best regards,
> > Vitor Soares
> > 
> 
> -- 
> -- 
> Regards,
> Przemek

Honestly, I think it is not right you being involved in this especially 
by the companies we represent.
As I already said before I here to contribute and if I think my solution 
is beneficial for everyone and doesn't have a negative impact on others 
drivers, I can't accept unjustifiable requirements.

Best regards,
Vitor Soares
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH] i3c: make sure the PID is set before registering the device

[Przemysław Gaj]

Hi Vitor,

The 12/10/2019 15:23, Vitor Soares wrote:
> 
> Hi Przemysław,
> 
> From: Przemysław Gaj <pgaj@cadence.com>
> Date: Tue, Dec 10, 2019 at 14:42:33
> 
> > Hi Vitor,
> > 
> > The 12/10/2019 14:28, Vitor Soares wrote:
> > > 
> > > Hi Boris,
> > > 
> > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > Date: Mon, Dec 09, 2019 at 12:26:16
> > > 
> > > > On Mon, 9 Dec 2019 12:20:03 +0000
> > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > > 
> > > > > Hi Boris,
> > > > > 
> > > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > Date: Mon, Dec 09, 2019 at 09:47:11
> > > > > 
> > > > > > On Sun, 8 Dec 2019 15:18:34 +0100
> > > > > > Przemysław Gaj <pgaj@cadence.com> wrote:
> > > > > >   
> > > > > > > From: Przemyslaw Gaj <pgaj@cadence.com>
> > > > > > > 
> > > > > > > Provisioned ID (PID) is the value with which device drivers are
> > > > > > > matched. I check the value before registering the device.
> > > > > > >   
> > > > > > 
> > > > > > Can this situation (having a device with a DA but without a valid PID)
> > > > > > happen right now or is this something you need to support secondary
> > > > > > masters who receive device DA (through DEFSVLS) without being able to
> > > > > > query extra information until they own the bus?  
> > > > > 
> > > > > This is the use case where a device fails the 
> > > > > i3c_master_pre_assign_dyn_addr() and I already mention it on [1].
> > > > > I still think the best way is to detach/free the devices that fails 
> > > > > during i3c_master_pre_assign_dyn_addr().
> > > > 
> > > > And I disagree (I gave my arguments already, so won't repeat them
> > > > here).
> > > 
> > > Sorry Boris, did you give? I ask you several times why to keep non DA 
> > > devices attached to the bus, yet you wasn't able to give me a technical 
> > > reason. Even from device model you should kept them.
> > 
> > I think we should keep them because framework should still have the
> > information about all the devices. We already discussed that, right?
> > Especially, when we have to deal with group addresses soon, it's better
> > to keep them.
> 
> Could you please point me where do we need it? We still have the 
> boardinfo that hold DT information which is what we need.

As MIPI 1.1 intruduces group address feature, I think it's better to
keep the devices on the list as the information about that device may be
needed when we manage devices under that group. I know the device is not
addressed that time but when it joins, we'll have the description
(possibly with group virtual I3C address) so we can assign it easily.

> 
> If a device doesn't have DA, why do we need it? By definition the i3c dev 
> desc is the discovered devices, if it doesn't have an DA isn't discovered 
> yet.

Yes, I know. MIPI spec defines that we should not define slave list
without DA but having devices or not in the subsystem is our internal
implementation and I think we can keep them and not serve them on the bus.
At least for now, when some big features are missing.

> 
> > 
> > > Honestly, I'm starting ask myself if this is something against because 
> > > every time that I'm trying to improve something I just see difficulty 
> > > from your side.
> > 
> > Don't forget that my patches are accepted after 6th, 7th versions. I
> > think that it just should work like that. This makes things better :)
> 
> I don't have issue in send multiple versions of my patches as long as 
> they are improved. In this case,  I don't agree with Boris's requirements 
> because in my tests I still have issues.
> Please make the exercise and check what needs to be done when a device 
> fails in i3c_master_pre_assign_dyn_addr() so everything goes ok and what 
> it is the advantage of kept i3c_dev_desc from those devices.

I forced the error of SETDASA and then joined the bus. Dupplicate was
found and that's it. What kind of issue you have? I can debug it on my
end and see if anything new is discovered.

It this problem is related to lack of slots in the HW, you can reuse
already allocated slot in ->attach_i3c_dev() and that should solve
the problem, right?

> 
> And still I have issue on my side. 
> 
> Did you test the previous patch? Did you find any issue? If so, you 
> wouldn't need the fix the i3c_master_register_new_i3c_devs() which is the 
> last instance of registering an i3c device.

I know, it's more like I'm preparing this for the next stage, mastership
takeover. I can introdue this also when introducing that patchset but I
want to keep mastership as simple as possible. I hope you understand
that. This should be beneficial for all of us.

> I would ask you to verify in v1.1 spec the CCCs table and verify if fix 
> the i3c_master_register_new_i3c_devs() is correct according what is 
> there.

Could you be more specific here? When you look into MIPI spec, you'll
see that we can have 'partialy discovered' devices by lack of information
in DEFSLVS.

> 
> > 
> > > 
> > > > Can we move on please?
> > > 
> > > I think you should start learning to listen the other and have the 
> > > greatness to accept the others experience and opinions. With your 
> > > experience you should already learn that.
> > > The previous patch makes all the sense yet your response was - I am not 
> > > convinced. Could you please give a proper justification? At least test 
> > > what I spend time to do?
> > > 
> > > > Can you send a new version that does what
> > > > I suggest, or should I ask Przemek to do it?
> > > 
> > > Does the ethics approve such attitude? Don't you see with kind of 
> > > attitude you are just putting people away of this subsystem?
> > > 
> > > > 
> > > > _______________________________________________
> > > > linux-i3c mailing list
> > > > linux-i3c@lists.infradead.org
> > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Di3c&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=cXyPCHO2vhcyD3aN-LnqOqzodbB9g-utJXeWOQRkJmk&s=odrtYhb-IaK-dfTzi1p7TwH2YJcnZ3RSdNNrKJjXFdM&e= 
> > > 
> > > Best regards,
> > > Vitor Soares
> > > 
> > 
> > -- 
> > -- 
> > Regards,
> > Przemek
> 
> Honestly, I think it is not right you being involved in this especially 
> by the companies we represent.

Oh, we are part of the community here, let's not speak about the
companies :)

> As I already said before I here to contribute and if I think my solution 
> is beneficial for everyone and doesn't have a negative impact on others 
> drivers, I can't accept unjustifiable requirements.



> 
> Best regards,
> Vitor Soares

-- 
-- 
Regards,
Przemek

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH] i3c: make sure the PID is set before registering the device

[Vitor Soares]

Hi Przemysław,

From: Przemysław Gaj <pgaj@cadence.com>
Date: Wed, Dec 11, 2019 at 09:33:18

> Hi Vitor,
> 
> The 12/10/2019 15:23, Vitor Soares wrote:
> > 
> > Hi Przemysław,
> > 
> > From: Przemysław Gaj <pgaj@cadence.com>
> > Date: Tue, Dec 10, 2019 at 14:42:33
> > 
> > > Hi Vitor,
> > > 
> > > The 12/10/2019 14:28, Vitor Soares wrote:
> > > > 
> > > > Hi Boris,
> > > > 
> > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > Date: Mon, Dec 09, 2019 at 12:26:16
> > > > 
> > > > > On Mon, 9 Dec 2019 12:20:03 +0000
> > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > > > 
> > > > > > Hi Boris,
> > > > > > 
> > > > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > > Date: Mon, Dec 09, 2019 at 09:47:11
> > > > > > 
> > > > > > > On Sun, 8 Dec 2019 15:18:34 +0100
> > > > > > > Przemysław Gaj <pgaj@cadence.com> wrote:
> > > > > > >   
> > > > > > > > From: Przemyslaw Gaj <pgaj@cadence.com>
> > > > > > > > 
> > > > > > > > Provisioned ID (PID) is the value with which device drivers are
> > > > > > > > matched. I check the value before registering the device.
> > > > > > > >   
> > > > > > > 
> > > > > > > Can this situation (having a device with a DA but without a valid PID)
> > > > > > > happen right now or is this something you need to support secondary
> > > > > > > masters who receive device DA (through DEFSVLS) without being able to
> > > > > > > query extra information until they own the bus?  
> > > > > > 
> > > > > > This is the use case where a device fails the 
> > > > > > i3c_master_pre_assign_dyn_addr() and I already mention it on [1].
> > > > > > I still think the best way is to detach/free the devices that fails 
> > > > > > during i3c_master_pre_assign_dyn_addr().
> > > > > 
> > > > > And I disagree (I gave my arguments already, so won't repeat them
> > > > > here).
> > > > 
> > > > Sorry Boris, did you give? I ask you several times why to keep non DA 
> > > > devices attached to the bus, yet you wasn't able to give me a technical 
> > > > reason. Even from device model you should kept them.
> > > 
> > > I think we should keep them because framework should still have the
> > > information about all the devices. We already discussed that, right?
> > > Especially, when we have to deal with group addresses soon, it's better
> > > to keep them.
> > 
> > Could you please point me where do we need it? We still have the 
> > boardinfo that hold DT information which is what we need.
> 
> As MIPI 1.1 intruduces group address feature, I think it's better to
> keep the devices on the list as the information about that device may be
> needed when we manage devices under that group. I know the device is not
> addressed that time but when it joins, we'll have the description
> (possibly with group virtual I3C address) so we can assign it easily.

Although you present this use case, in practice you won't need the 
i3c_dev_desc allocated because I have introduced 
i3c_master_init_i3c_dev_boardinfo() in this patch series.
if you check the i3c_master_add_i3c_dev_locked() this i3c_dev_desc that 
we are keeping unnecessarily is detach and free later.
So I wonder why do we need to wait and waste resources until there? 

> 
> > 
> > If a device doesn't have DA, why do we need it? By definition the i3c dev 
> > desc is the discovered devices, if it doesn't have an DA isn't discovered 
> > yet.
> 
> Yes, I know. MIPI spec defines that we should not define slave list
> without DA but having devices or not in the subsystem is our internal
> implementation and I think we can keep them and not serve them on the bus.
> At least for now, when some big features are missing.

Sorry, you are only presenting an assumption of something that I already 
presented facts that is problematic and a solution to fix it.

> 
> > 
> > > 
> > > > Honestly, I'm starting ask myself if this is something against because 
> > > > every time that I'm trying to improve something I just see difficulty 
> > > > from your side.
> > > 
> > > Don't forget that my patches are accepted after 6th, 7th versions. I
> > > think that it just should work like that. This makes things better :)
> > 
> > I don't have issue in send multiple versions of my patches as long as 
> > they are improved. In this case,  I don't agree with Boris's requirements 
> > because in my tests I still have issues.
> > Please make the exercise and check what needs to be done when a device 
> > fails in i3c_master_pre_assign_dyn_addr() so everything goes ok and what 
> > it is the advantage of kept i3c_dev_desc from those devices.
> 
> I forced the error of SETDASA and then joined the bus. Dupplicate was
> found and that's it. What kind of issue you have? I can debug it on my
> end and see if anything new is discovered.
> 
> It this problem is related to lack of slots in the HW, you can reuse
> already allocated slot in ->attach_i3c_dev() and that should solve
> the problem, right?
> 
> > 
> > And still I have issue on my side. 
> > 
> > Did you test the previous patch? Did you find any issue? If so, you 
> > wouldn't need the fix the i3c_master_register_new_i3c_devs() which is the 
> > last instance of registering an i3c device.
> 
> I know, it's more like I'm preparing this for the next stage, mastership
> takeover. I can introdue this also when introducing that patchset but I
> want to keep mastership as simple as possible. I hope you understand
> that. This should be beneficial for all of us.
> 
> > I would ask you to verify in v1.1 spec the CCCs table and verify if fix 
> > the i3c_master_register_new_i3c_devs() is correct according what is 
> > there.
> 
> Could you be more specific here? When you look into MIPI spec, you'll
> see that we can have 'partialy discovered' devices by lack of information
> in DEFSLVS.

Please check the table and you will realize that the fix isn't 
compatible.
Sorry but for now this is all. 

Best regards,
Vitor Soares


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c