On Thu, Jul 19, 2018 at 04:10:50PM +0100, Andre Przywara wrote: > Hi, > > On 19/07/18 15:46, Maxime Ripard wrote: > > On Thu, Jul 19, 2018 at 02:35:54PM +0100, Andre Przywara wrote: > >> Recent Linux versions refuse to print actual virtual kernel addresses, > >> to not give a hint about the location of the kernel in a randomized virtual > >> address space. This affects the output of the sunxi MMC controller > >> driver, which now produces the rather uninformative line: > >> > >> [ 1.482660] sunxi-mmc 1c0f000.mmc: base:0x(____ptrval____) irq:8 > >> > >> Since the virtual base address is not really interesting in the first > >> place, let's just drop this value. The same applies to Linux' notion of > >> the interrupt number, which is independent from the GIC SPI number. > >> We have the physical address as part of the DT node name, which is way > >> more useful for debugging purposes. > >> To keep a success message in the driver, we print some information that > >> is not too obvious and that we learned while probing the device: > >> the maximum request size and whether it uses the new timing mode. > >> So the output turns into: > >> sunxi-mmc 1c0f000.mmc: max request size: 16384 KB, uses new timings mode > >> sunxi-mmc 1c11000.mmc: max request size: 2048 KB > >> > >> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx> > >> --- > >> Changelog v1 ... v2: > >> - dropped output of Linux interrupt number > >> - added max request size and timings mode output > >> > >> drivers/mmc/host/sunxi-mmc.c | 5 ++++- > >> 1 file changed, 4 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c > >> index 8e7f3e35ee3d..fbbc09d82338 100644 > >> --- a/drivers/mmc/host/sunxi-mmc.c > >> +++ b/drivers/mmc/host/sunxi-mmc.c > >> @@ -1407,7 +1407,10 @@ static int sunxi_mmc_probe(struct platform_device *pdev) > >> if (ret) > >> goto error_free_dma; > >> > >> - dev_info(&pdev->dev, "base:0x%p irq:%u\n", host->reg_base, host->irq); > >> + dev_info(&pdev->dev, "max request size: %u KB%s\n", > >> + mmc->max_req_size >> 10, > >> + host->use_new_timings ? ", uses new timings mode" : ""); > > > > I really don't know how to feel about this one. > > > > This isn't more useful to the regular user wanting to see if the > > driver is probed, which is what this message should be about. > > > > And this one isn't clearer or more obvious than the previous one > > (which was already pretty bad). I really think having some message > > that basically says "MMC controller initialized" or something along > > those lines would work better. > > I see your point, and am happy to change that. > On the other hand there might be people that complain about chatty > drivers, and printing a line just to says "Success!!!!!" is a bit BSP > kernel like ;-). Well, we're currently printing copyright notices, so I guess we're worse than the BSPs :) > That dmesg line could as well be used to print something useful, and > transport the "success" message along with that. So basically I > scanned the probe function for some information that would justify > an output (something non-obvious or information probed), to avoid > dropping this at all (as you initially said yesterday). > MMC_CAP_1_8V_DDR was another candidate, btw. > > > However, I can also see value in having this printed, for developers, > > but maybe as dev_dbg? > > I think it's useful to have this in non-debug kernels as well, because > this is what people tend to use. And this would allow developers to much > easier debug user problems, for instance when they create board DTs: > When this line doesn't appear, there might be a regulator missing, for > instance. If it's there, the MMC driver is happy and we have one less > thing to worry about. > > So why not combine the benefit of the success message and the > information for developers, if we have that one line of output anyway? > I think we have way more chattier drivers and way more cryptic messages > in the dmesg, so a single line with some technical details does not > hurt, especially if we have that already. > > But I don't have a strong opinion on that, so leave it up for Ulf and > you to decide: keep this patch (or print some other info); replace the > output with a success message or drop the line at all. How about both then? Something like "initialized with %d request size and new timings\n" ? It makes it obvious enough for someone that doesn't really has an idea, while outputting the proper informations you wanted. Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
Attachment:
signature.asc
Description: PGP signature
