Hi,

On 02/19/2014 05:25 PM, Tejun Heo wrote:

Hello, Hans.

On Wed, Feb 19, 2014 at 04:26:43PM +0100, Hans de Goede wrote:

quoted

The devres usage are really 2 different issues:

1) There is the issue that we're getting clocks by index (this is by design as
clk-names are not generic), but there is no devm_get_clk_by_index (or some
such), this is a shortcoming of the clk-core, which needs a separate patch
to fix. I would rather not delay this patch-set based on getting a patch
into another subsys.

Yes, that'd be my usual preference too.  The only reason I'm hesitant
is because nobody seems to be too interested in long term aspect of
the code base and the only leverage I have seems to be rejecting
drivers until things become right.

quoted

Note that even with devm_get_clk_by_index we can unfortunately not get rid of
ahci_platform_put_resources() as in Roger's follow-up patches it gets used for
putting some runtime pm related resources too.

I guess this argues for simply turning ahci_platform_get_resources into a
devm_ahci_platform_get_resources doing its own devm handling, and then
making ahci_platform_put_resources a private function called by the devm
framework. If you agree I can do that for the next patch-set.

Note that libata core already does something similar.  Please take a
look at ata_host_alloc() for details.  You don't really need to create
a separate devm_ prefixed variant.  Just making the function register
devres automatically should be enough.  If this is too much work, I'm
okay with deferring it until later if you promise to do it soonish.

Ok, I'll try to do this for the next revision.

quoted

2) In some comments you also seem to want devm variants of enable / disable
resources, or at least have ahci_platform_put_resources do the disable
automatically. The problem is that most of the functions called here need to
be balanced, they increment / decrement usage counters in the clk / regulator
subsystems, so we cannot simply unconditional do an ahci_platform_disable_resources
in ahci_platform_put_resources

Doing the disable automatically requires tracking the enable state, and doing this
per resource, since the whole idea of having a separate ahci_platform_enable_resources
is that some drivers may want to override its behavior doing things in a different
order.

To ensure proper tracking we would then need to offer ahci_platform_enable_regulator,
etc. functions, and ensure that all drivers using the ahci_platform framework
always go through these, rather then calling directly into the relevant framework.

This is all doable, and I'm not against doing this but before spending a couple of
hours coding this all up, I would like to hear back from you whether you would like
to see this, or would rather keep things as is.

Yes, in principle, I want every resource used by ahci_platform to be
devres managed.  I *think* devres should be flexible enough to handle
what you're describing (each devres resource can have data for state
tracking and devres resources can be looked up and modified, so the
different orders should be okay) but if not I'd be happy to help
extend it so that it can.

Most of the resources are already devres managed (I use devm functions
to get them), the problem is not in freeing our reference to the resources,
the problem is that we've sequences like this:

devm_get_foo
enable_foo
disable_foo
(automatic release foo)

Where enable / disable can be done repeatedly (ie each suspend / resume).

From your review comments, I take it that you want the final disable_foo

on driver release to happen automatically.

My preference for this would be to extend the devres tracking already present
in the relevant subsystems to keep track of the enable count done through a
specific reference, to allow automatic disable (if needed) on release.

But thinking more about this, I think that doing this automatically is a bad
idea, because then we fixate the shutdown sequence to a certain order (the
order in which we did the _get_foo for the resources) and the correct order may
be device specific.

So TL;DR: Yes to making things so that ahci_platform_put_resources gets done
automatically, no to automating the disable calls.

If I don't hear back from you, then I'll respin the patch-set assuming that
you agree to the above.

Regards,

Hans