linux-next: "amba: Remove AMBA level regulator support" commit.

Hi Mark,

The commit in linux-next:

-----------------
commit b2bc9e0ee349db84035d8141e85babc7986594d7

Author: Mark Brown <[email protected]>
Date:   Mon Apr 2 12:30:35 2012 +0100

ARM: 7366/1: amba: Remove AMBA level regulator support
-----------------

is pointed at by git bisect for the u8500_defconfig build failure:

drivers/dma/ste_dma40.c:2708:3: error: implicit declaration of function 'regulator_disable' [-Werror=implicit-function-declaration] drivers/dma/ste_dma40.c:2747:3: error: implicit declaration of function 'regulator_enable' [-Werror=implicit-function-declaration] drivers/dma/ste_dma40.c:3260:3: error: implicit declaration of function 'regulator_get' [-Werror=implicit-function-declaration] drivers/dma/ste_dma40.c:3271:4: error: implicit declaration of function 'regulator_put' [-Werror=implicit-function-declaration]

http://kisskb.ellerman.id.au/kisskb/buildresult/6095691/

If, like my earlier mail this week, this is something you've already fixed, then please provide me a link, and I'll gladly include that in the triage report, so you don't get needless extra mails about it.

Thanks,
Paul.
--

git bisect start
# good: [0034102808e0dbbf3a2394b82b1bb40b5778de9e] Linux 3.4-rc2
git bisect good 0034102808e0dbbf3a2394b82b1bb40b5778de9e
# bad: [87487c4514a94892ce478dc012da6d12a98ae360] Add linux-next specific files for 20120412
git bisect bad 87487c4514a94892ce478dc012da6d12a98ae360
# bad: [45701d47f211d6ce72ee2c1725904d2e173eb29f] Merge remote-tracking branch 'sound/for-next'
git bisect bad 45701d47f211d6ce72ee2c1725904d2e173eb29f
# good: [5d2acf8cc7515129a5d3aed7b0e0be18dbcf524f] Merge branch 'topic/misc' into for-next
git bisect good 5d2acf8cc7515129a5d3aed7b0e0be18dbcf524f
# bad: [846c4b315b6faae8f5ede661d80c222d669adbda] Merge remote-tracking branch 'target-merge/for-next-merge'
git bisect bad 846c4b315b6faae8f5ede661d80c222d669adbda
# bad: [9235a323f99492120565935040b6be61e687d0c6] Merge remote-tracking branch 'gfs2/master'
git bisect bad 9235a323f99492120565935040b6be61e687d0c6
# bad: [6c944bb0c84fa1a03af503e926d7bd3a4a7e7376] Merge remote-tracking branch 'usb.current/usb-linus'
git bisect bad 6c944bb0c84fa1a03af503e926d7bd3a4a7e7376
# bad: [ef75674b6700ad96cac1e1c377cf9809cea6a369] Merge remote-tracking branch 'net/master'
git bisect bad ef75674b6700ad96cac1e1c377cf9809cea6a369
# good: [39f86a608a3e0f0164bd1540acf87696cfdfb5bb] Merge branch 'v4l_for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
git bisect good 39f86a608a3e0f0164bd1540acf87696cfdfb5bb
# good: [a1ada086062101533eb0f841d3884137688091ec] Merge tag 'sound-3.4' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
git bisect good a1ada086062101533eb0f841d3884137688091ec
# good: [87151b8689d890dfb495081f7be9b9e257f7a2df] net: allow pskb_expand_head() to get maximum tailroom
git bisect good 87151b8689d890dfb495081f7be9b9e257f7a2df
# good: [f549e088b806f44b6ab6eeef0cb71ced1d2488dd] Merge tag 'rdma-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/roland/infiniband
git bisect good f549e088b806f44b6ab6eeef0cb71ced1d2488dd
# bad: [b2bc9e0ee349db84035d8141e85babc7986594d7] ARM: 7366/1: amba: Remove AMBA level regulator support
git bisect bad b2bc9e0ee349db84035d8141e85babc7986594d7
# good: [34af657916332e89564566bc8d35e3e06cc0c236] ARM: 7377/1: vic: re-read status register before dispatching each IRQ handler git bisect good 34af657916332e89564566bc8d35e3e06cc0c236

That's because Mark's patch removes "#include <linux/regulator/consumer.h>" included indirectly by drivers/dma/ste_dma40.c from linux/amba/bus.h. The ste_dma40.c should really directly includes consumer.h.

Regards,
Shawn

Okay, I'll drop this patch because it's causing regressions, so it can't be pushed as a 'fix' during -rc.

I did say we should've been pushing Shawn's patch in as a minimal fix for 3.4... Alternatively if you're happy with the code just keeping the header in place should avoid any issues, the one regression was just an implicit header dependency.

Regardless of what happens for 3.4 we should keep the removal for -next,
it's clear that we don't want the bus doing this and it's causing
breakage for the non-ST platforms. Do I need to resend the patch to the
patch system or will the existing copy be OK?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120413/34135552/attachment.sig>

What patch?

What I want is something that doesn't cause a regression for 3.4.

The original one which just changed the return code that's checked for.

Well, if the overall result is something other than removing the code in
3.5 then we'll just get further regressions down the line.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120413/f4fac225/attachment.sig>

I guess you are referring to this one:
http://www.spinics.net/lists/arm-kernel/msg167196.html

Oh, and note that there's already a fix for the missing header although
with all the git trees I've no idea what the process is for getting it
into mainline.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120413/c9690b88/attachment.sig>

That really doesn't make any difference about whether I can push 7366/1 as is or not. If the fix for the missing header is going through some other git tree, I can't push 7366/1 until that fix has gone in - and I'm not going to be tracking when that happens.

TBH, its something that _you_ need to manage - you created this regression in the first place by changing the regulator API without first reviewing all the callsites. Grep is a wonderful tool for finding those. So I'll leave it entirely up to you to figure out how to fix the AMBA regression you caused in a sane way in -rc - and without causing any additional regressions by doing so.

If you want me to apply 7367/1 instead, then please say so directly. If you want 7366/1 plus the header file fixed, then that needs to be figured out.

I've got quite limited visibility on how this stuff gets handled; the patch tracking system always feels like a bit of a black hole to me as I'm never sure when to send things to it or when they might get applied and the usual mechanisms for indicating how things get applied (tags in the header and comments after the ---) aren't available.

I have to say was rather surprised when I realised you had applied my change for -rc rather than for -next.

You know as well as I do that a grep of all the users isn't going to turn up everything reliably, this is one of the reasons why people should be testing -next (which apparently nobody had been doing on any of the affected platforms).

Looking at the code I suspect that my initial read was that if anything hit that case we'd crash as vcore is left with an error pointer, though unusually for such code the IS_ERR() is actually used at every call site so it actually managed to work. Plus the fact that the code was never supposed to work in the first place, of course.

Right from my initial reply to it I've said that 7367/1 was a good, minimal fix for 3.4 but that for 3.5 we should be doing 7366/1 or thereabouts. Like I say I was quite surprised when you applied the larger fix for 3.4, though there are good arguments in favour of doing that in that it removes an API which nobody should be using. So long as what we end up with in 3.5 is 7366/x (which I think everyone agrees is the goal) I'm not too fussed about which solution is adopted for 3.4.

There's two ways to go about this:

- Apply 7366/3 (which keeps regulator/consumer.h in linux/amba.h but is otherwise the same as 7366/1) and then either leave things or clean up the header in 3.5.

- Apply 7367/1 for 3.4 and then rebase 7366/x after it and put that in -next for inclusion in 3.5.

Which one is chosen is a matter of taste for you. I'd initially expected the second approach but as it seems you're comfortable with the larger patch we may as well go with it, the header file should be the only dependency.

For the benefit of people reading the mails 7366/x is the change to
remove the regulator usage from AMBA (amba: Remove AMBA level regulator
support) and 7367/x is the change to just change the return code (amba:
adapt to regulator probe deferral change).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120413/ad988d8c/attachment-0001.sig>

Actually *not* having Marks patch also causes a regression on Ux500, albeit not at compile time, but at runtime.

So for me as ux500 maintainer it's perfectly OK to have compilation broken until that DMA patch arrives (should just be an issue of the DMA maintainer picking it up), but if others feel differently by all means hold it off until the other one arrives.

Yours,
Linus Walleij

Not me, but Linus W has a patch. No idea about a link off the top of my
head, google should turn something up I guess. It's exactly the same
sort of implicit header dependency issue that your cleanups keep
triggering.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120413/9f2a6a67/attachment.sig>

I've already sent a patch to the DMA maintainer to fix this. It was a bug in the DMA driver actually - it did not include the proper headers - so it just got triggered by this patch.
http://marc.info/?l=linux-kernel&m=133424730706377&w=2

Yours,
Linus Walleij