05:40 <mvaittin> ukleinek: out of the curiosity - isn't a warning in a log for a 'remove failure' seen worthy? I guess that could be a hint that there may be something to fix in a driver?

06:53 <wens> robher: is it possible to describe two DT properties as mutually exclusive? I tried having "oneOf" under properties and it wasn't accepted.

07:32 <ukleinek> mvaittin: returning an error code isn't a sane way to handle an error.

07:32 <ukleinek> (in this case that is)

07:33 <ukleinek> mvaittin: Most drivers that I fixed to return 0 unconditionally and obviously in .remove either already did return 0 or had wrong expectations and created a resource leak.

07:35 <mvaittin> ukleinek: I think returning error has benefits. At least having logging of failure centered in one place instead of adding prints in all of the individual drivers. Not going to argue that though - just wanted to know why you think the return value should be removed && got the response - thanks.

07:36 <ukleinek> mvaittin: see commits like 61b738e938ef091e0c4c4e8eadeaf27502732e97 or 5d354dc35ebb0b224d627264c60f60dbda3a1bc3

07:40 <ukleinek> mvaittin: regarding the error log I have two concerns: a) nobody reads the error log anyhow; and b) If there is a problem and the driver just returns (say) -EIO, you get "yourdev: Failed to unbind driver (-EIO), ignoring". Something like "yourdev: Failed to disable clk (-EIO)" is muuuch more valuable.

07:41 <ukleinek> see for example 48f45c2a969b5a012fda468350706c3a479f0707

07:42 <ukleinek> (this is about i2c, but in this regard it the same)

08:24 <mvaittin> ukleinek: regarding the two concerns - I feel a) is false statement. I for example do read logs. And I bet quite a few others do so too. Especially if something does not work as expected. Regarding b) - the "Failed to unbind driver" is still _far_ better than silently failing. After that one has a driver which he can further study. (It seems I started arguing anyways ;) )

08:25 <mvaittin> "Failed to disable clk" is for sure better for debugging - but also adds the print footprint.

08:25 <mvaittin> because that needs to be added to every spi driver

08:26 <geertu> wens: yes, but you have to move the "oneOf" outside the properties, i.e. at top indentation

08:27 <geertu> See e.g. interruots{,-extended} in Documentation/devicetree/bindings/timer/arm,arch_timer.yaml

08:29 <mvaittin> By the way, I checked the 61b738e938ef091e0c4c4e8eadeaf27502732e97 . I don't see your concern of not freeing the resource there :) (If the removed checks can never trigger - then resources never leak. If the checks would trigger (for example as a result of some spi-core change) - then the resource freeing would not be possible. But yes - I see why you removed those code lines and I agree that change is reasonable - just don't share your concern

08:29 <mvaittin> with the leak there

08:33 <ukleinek> mvaittin: leaking resources was not a problem everywhere

08:34 <ukleinek> and for a good error message the overhead is justified

09:05 <mvaittin> ukleinek: I agree that for a good error message we can in many cases accept the overhead. Yet, in many cases the return value and source gives you the same information. Removes in drivers rarely have that many error branches (my gut feeling - no proper statistics to back that up). Anyways - the worst scenario is that we have neither specific error message from driver, nor generic one from the framework. And in my (limited) experience the real

09:05 <mvaittin> world developers are damn lazy adding the prints - but far better in using error codes in return values. I don't think you find that many remove() functions with accurate error prints... Well, I feel this is not much of my business though - as I said, I just wanted to hear the rationale for dropping the returnvalue

09:07 <ukleinek> mvaittin: most remove functions are sane and there is nothing that can go wrong

09:11 <mvaittin> ukleinek: Thanks for the patience :) I am not fully buying this - but luckily there are more clever people than me taking care of the decisions like this one ;)

17:47 <alexels> quit