OMAP2+ hwmod fixes

_init_clock always returns 0 and does
not propogate the error (in case of failure)
back to the caller, causing _init_clocks to
fail silently.

Signed-off-by Rajendra Nayak<[email protected]>

---
  arch/arm/mach-omap2/omap_hwmod.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/mach-omap2/omap_hwmod.c +0 -0

index cd9dcde..960461f 100644

Add a hwmod state check in the _setup function
to avoid setting up hwmods' for which clock
lookup has failed.

Signed-off-by Rajendra Nayak<[email protected]>

Add a hwmod state check in the _setup function
to avoid setting up hwmods' for which clock
lookup has failed.

Signed-off-by Rajendra Nayak<[email protected]>

---
  arch/arm/mach-omap2/omap_hwmod.c |    6 ++++++
  1 files changed, 6 insertions(+), 0 deletions(-)

arch/arm/mach-omap2/omap_hwmod.c +0 -0

index e282e35..cd9dcde 100644

This series fixes some hwmod api return values and also adds some state checks.
The hwmod iterator functions are made to continue and not break if one of the callback functions ends up with an error.

-----Original Message-----
From: Cousson, Benoit [mailto:b-cousson at ti.com]
Sent: Wednesday, February 16, 2011 6:37 PM
To: Nayak, Rajendra
Cc: linux-omap at vger.kernel.org; paul at pwsan.com;

Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes

Hi Rajendra,

Quoted 5 lines · Rajendra

This series fixes some hwmod api return values and also adds some state checks.
The hwmod iterator functions are made to continue and not break if one of the callback functions ends up with an error.

By doing that, you change the behavior of this function. I'm not sure I fully understand why.
Could you elaborate on the use case?

To avoid that behavior in the past, I was just returning 0 in case of failure to avoid stopping the iteration. It looks like you do not want to stop the iteration but still retrieve the error.
I do not see in this series what you plan to do with the error at the end of the iteration.

Thanks,
Benoit

Hi Benoit,

Quoted 5 lines

-----Original Message-----
From: Cousson, Benoit [mailto:b-cousson at ti.com]
Sent: Wednesday, February 16, 2011 6:37 PM
To: Nayak, Rajendra
Cc: linux-omap at vger.kernel.org; paul at pwsan.com;

linux-arm-kernel at lists.infradead.org

Quoted 11 lines

Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes

Hi Rajendra,

Quoted 5 lines · Rajendra

This series fixes some hwmod api return values and also adds some state checks.
The hwmod iterator functions are made to continue and not break if one of the callback functions ends up with an error.

Quoted 3 lines

By doing that, you change the behavior of this function. I'm not sure I fully understand why.
Could you elaborate on the use case?

Since these functions iterate over all hwmods calling a common callback function, there might be cases wherein the callback function for *some* hwmods might fail. For instance, if you run through all hwmods and try to clear the context registers for all of them, for some hwmods which might not have context registers the callback function might return a -EINVAL, however that should not stop you from attempting to clear the context

registers for the rest of the hwmods which have them and abort the function midway, no? This is more hypothetical, however the real usecase that prompted me with this patch was when I had some wrong state check in _setup function, and the iterator would stop with the first failure and not even attempt to setup the rest of the hwmods.

Quoted 6 lines

To avoid that behavior in the past, I was just returning 0 in case of failure to avoid stopping the iteration. It looks like you do not want to stop the iteration but still retrieve the error.
I do not see in this series what you plan to do with the error at the end of the iteration.

Most users of these iterators would just use the non-zero return value to throw an error/warning out stating there were failures running through all the callback functions. That does not change with this patch, and they can still do it.

Quoted 30 lines · Rajendra

Quoted 5 lines

-----Original Message-----
From: Cousson, Benoit [mailto:b-cousson at ti.com]
Sent: Wednesday, February 16, 2011 6:37 PM
To: Nayak, Rajendra
Cc: linux-omap at vger.kernel.org; paul at pwsan.com;

linux-arm-kernel at lists.infradead.org

Quoted 11 lines

Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes

Hi Rajendra,

Quoted 5 lines · Rajendra

This series fixes some hwmod api return values and also adds some state checks.
The hwmod iterator functions are made to continue and not break if one of the callback functions ends up with an error.

Quoted 3 lines

By doing that, you change the behavior of this function. I'm not sure I fully understand why.
Could you elaborate on the use case?

Since these functions iterate over all hwmods calling a common callback function, there might be cases wherein the callback function for *some* hwmods might fail. For instance, if you run through all hwmods and try to clear the context registers for all of them, for some hwmods which might not have context registers the callback function might return a -EINVAL, however that should not stop you from attempting to clear the context

Quoted 8 lines

registers for the rest of the hwmods which have them and abort the function midway, no? This is more hypothetical, however the real usecase that prompted me with this patch was when I had some wrong state check in _setup function, and the iterator would stop with the first failure and not even attempt to setup the rest of the hwmods.

Yeah, but by using that function you implicitly accept that an error will break the loop, so the function you pass to the iterator should be written for that. Meaning if you do not want to exit on error you should not report an error.

Quoted 7 lines

Quoted 6 lines

To avoid that behavior in the past, I was just returning 0 in case of failure to avoid stopping the iteration. It looks like you do not want to stop the iteration but still retrieve the error.
I do not see in this series what you plan to do with the error at the end of the iteration.

Quoted 5 lines

Most users of these iterators would just use the non-zero return value to throw an error/warning out stating there were failures running through all the callback functions. That does not change with this patch, and they can still do it.

Except that now, the iterator is not able anymore to stop on error if needed potentially.
My point is that you are changing the behavior of this function without maintaining the legacy.

So maybe creating a new iterator is a better approach.
Even is this feature is not used today I have some secret plan for this behavior in the near future :-)

But my initial comment is still valid, if you do not care about the final error status after the iteration, you'd better not return any error at the beginning.
This was the behavior or the _setup until now.

-----Original Message-----
From: Paul Walmsley [mailto:paul at pwsan.com]
Sent: Monday, February 21, 2011 10:25 PM
To: Cousson, Benoit; Nayak, Rajendra
Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes

Hello Beno?t, Rajendra,

Quoted 41 lines · Benoit

Quoted 30 lines · Rajendra

Quoted 5 lines

-----Original Message-----
From: Cousson, Benoit [mailto:b-cousson at ti.com]
Sent: Wednesday, February 16, 2011 6:37 PM
To: Nayak, Rajendra
Cc: linux-omap at vger.kernel.org; paul at pwsan.com;

linux-arm-kernel at lists.infradead.org

Quoted 11 lines

Subject: Re: [PATCH 0/3] OMAP2+ hwmod fixes

Hi Rajendra,

Quoted 5 lines · Rajendra

This series fixes some hwmod api return values and also adds some state checks.
The hwmod iterator functions are made to continue and not break if one of the callback functions ends up with an error.

Quoted 3 lines

By doing that, you change the behavior of this function. I'm not sure I fully understand why.
Could you elaborate on the use case?

Since these functions iterate over all hwmods calling a common callback function, there might be cases wherein the callback function for *some* hwmods might fail. For instance, if you run through all hwmods and try to clear the context registers for all of them, for some hwmods which might not have context registers the callback function might return a -EINVAL, however that should not stop you from attempting to clear the context

Quoted 8 lines

registers for the rest of the hwmods which have them and abort the function midway, no? This is more hypothetical, however the real usecase that prompted me with this patch was when I had some wrong state check in _setup function, and the iterator would stop with the first failure and not even attempt to setup the rest of the hwmods.

Yeah, but by using that function you implicitly accept that an error

Quoted 1 line

break the loop, so the function you pass to the iterator should be

Quoted 1 line

that. Meaning if you do not want to exit on error you should not

Quoted 9 lines

error.

Quoted 7 lines

Quoted 6 lines

To avoid that behavior in the past, I was just returning 0 in case of failure to avoid stopping the iteration. It looks like you do not want to stop the iteration but still retrieve the error.
I do not see in this series what you plan to do with the error at the end of the iteration.

Quoted 7 lines

Quoted 5 lines

Most users of these iterators would just use the non-zero return value to throw an error/warning out stating there were failures running through all the callback functions. That does not change with this patch, and they can still do it.

Except that now, the iterator is not able anymore to stop on error if

Quoted 2 lines

potentially.
My point is that you are changing the behavior of this function

Quoted 4 lines

maintaining the legacy.

So maybe creating a new iterator is a better approach. Even is this feature is not used today I have some secret plan for

Quoted 3 lines

behavior in the near future :-)

But my initial comment is still valid, if you do not care about the

Quoted 1 line

error status after the iteration, you'd better not return any error at

Quoted 2 lines

beginning.
This was the behavior or the _setup until now.

The original behavior of the iterator was intentional: loops over functions like _init_clocks() and _setup() should terminate upon encountering an error. This is because the rest of that code currently relies on the hwmod/clock data to be accurate in order to work. There

no provision in the code for a hwmod to fail initialization due to data errors. The idea was that if the data is inaccurate, the data should be fixed first before doing anything else.

That said, I suppose that it's possible to enhance the code such that hwmods could be allowed to fail initialization, and simply brick themselves, rather than prevent the rest of the hwmods from

Probably that would need a new _HWMOD_STATE_INIT_FAILED, or something similar. If a hwmod would end up in that state, it must not be used by any other code, and the code should complain loudly.

The broader issue of whether the iterators should return immediately

failure (as the current code does), or continue through the rest of the list, is a separate one. I'd suggest one of two approaches:

1. If the rest of the code can be changed to gracefully handle cases

hwmod initialization fails, and if Beno?t agrees, I don't have a problem with changing the iterator behavior to ignore failures as you describe. Of course, all of the current users of omap_hwmod_for_each*() would need to be checked to ensure that this behavior makes sense for them.

... or ...

2. The iterators could take an extra parameter that would control the behavior upon encountering an error: terminate, or continue. But I am

sure that both cases are needed. Ideas, feedback here are welcome.

- Paul

The original behavior of the iterators, to terminate upon encountering an error, seems fine to me. The only problem I faced was that they fail silently and go undetected, unless their user catches the return value and WARN's, which I found was not the case with most users, mainly those of omap_hwmod_for_each_by_class.
I was thinking of keeping the behaviour of these iterators same for now and add WARN's in these iterators itself upon an error, so its seen even if the user fails to catch it.

-----Original Message-----
From: Paul Walmsley [mailto:paul at pwsan.com]
Sent: Wednesday, February 23, 2011 12:40 AM
To: Rajendra Nayak; Cousson, Benoit
Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: RE: [PATCH 0/3] OMAP2+ hwmod fixes

Hi Rajendra

Quoted 9 lines · Rajendra Nayak

The original behavior of the iterators, to terminate upon encountering an error, seems fine to me. The only problem I faced was that they fail silently and go undetected, unless their user catches the return value and WARN's, which I found was not the case with most users, mainly those of omap_hwmod_for_each_by_class.
I was thinking of keeping the behaviour of these iterators same for now and add WARN's in these iterators itself upon an error, so its seen even if the user fails to catch it.

What's your opinion on adding the pr_err() or WARN() into the code that the iterator calls for each hwmod? That code should know why something fails, so it should be able to provide a more detailed error message.

course, it is not as general a solution...

- Paul

Hi Paul,

Quoted 17 lines

From: Paul Walmsley [mailto:paul at pwsan.com]
Sent: Wednesday, February 23, 2011 12:40 AM

Hi Rajendra

Quoted 9 lines · Rajendra Nayak

The original behavior of the iterators, to terminate upon encountering an error, seems fine to me. The only problem I faced was that they fail silently and go undetected, unless their user catches the return value and WARN's, which I found was not the case with most users, mainly those of omap_hwmod_for_each_by_class.
I was thinking of keeping the behaviour of these iterators same for now and add WARN's in these iterators itself upon an error, so its seen even if the user fails to catch it.

Quoted 3 lines

What's your opinion on adding the pr_err() or WARN() into the code that the iterator calls for each hwmod? That code should know why something fails, so it should be able to provide a more detailed error message.

Of

Quoted 1 line

course, it is not as general a solution...

I agree, if the callback functions are written with proper errors or WARN's, they are the right place where most of the details' exist. So maybe we don't need these in the iterator's after all.

So to conclude, I will drop the #3 and just push #1 and #2.

#1 is fine with addition of the WARN.

#2 does return an error but does not print anything, but since each call (_init_main_clk, _init_interface_clks, _init_opt_clks) does report some pr_warn in case of error, this is fine.