On Tue, 2019-12-17 at 14:05 +0100, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Fri, Dec 13, 2019 at 01:04:20PM +0100, Christian Gromm wrote:
> > This patch moves the core module to the /drivers/most directory
> > and makes all necessary changes in order to not break the build.
> >
> > Signed-off-by: Christian Gromm <christian.gromm@xxxxxxxxxxxxx>
>
> I've applied the patches up to this one in the series, but I still
> have
> questions about the file you are trying to move here.
>
> It's not in this patch, but I'll just quote from the file
> drivers/staging/most/core.c directly:
>
> * Copyright (C) 2013-2015 Microchip Technology Germany II GmbH & Co.
> KG
>
> You've touched this file since 2015 :)
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> No need for this, You have drivers here, no need to use any pr_*
> calls,
> as you always have a device structure.
> Along with that, almost all of your pr_info() calls are really
> errors/warnigns, so use dev_err() or dev_warn() instead please.
>
> The one:
> pr_info("registered new core component %s\n", comp->name);
>
> Should at best be a dev_info() line, but really, you don't need to be
> loud if all goes well, right?
>
> pr_info("deregistering component %s\n", comp->name);
>
> Should be dev_dbg().
>
> static void release_interface(struct device *dev)
> {
> pr_info("releasing interface dev %s...\n", dev_name(dev));
> }
>
> static void release_channel(struct device *dev)
> {
> pr_info("releasing channel dev %s...\n", dev_name(dev));
> }
>
> How did I miss this before?
>
> The driver core documentation used to have a line saying I was
> allowed
> to make fun of programmers who did this, but that had to be removed
> :(
>
> Anyway, this is totally wrong, first off, delete the debugging lines.
> Secondly how are you really releasing anything?
Allocated memory is being freed inside the deregister* functions,
once a device is detached from the system or the physical adapter
driver has been removed. There a loop frees all channels and interfaces
and the devices are unregistered with the kernel.
I can move this to the release functions.
> You have to free the
> memory here. You can not have an "empty" release function, the
> driver
> core requires you to actually do something here.
>
> Same for release_most_sub() and anywhere else I missed in my review.
Here no memory has been allocated dynamically. What am I supposed to
free up?
I've seen code in the kernel that does host empty release functions.
That's probably why I didn't recognized this as a red flag or sensed
any bad feelings.
thanks,
Chris
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
