Re: [PATCH 3/3] staging: octeon: ethernet: add pr_fmt macro

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Mar 31, 2026 at 12:46:10PM +0530, Ayush Mukkanwar wrote:
> On Mon, Mar 30, 2026 at 9:26 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Mar 26, 2026 at 12:42:27AM +0530, Ayush Mukkanwar wrote:
> > > On Wed, Mar 25, 2026 at 2:41 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Wed, Mar 25, 2026 at 02:33:00PM +0530, Ayush Mukkanwar wrote:
> > > > > On Tue, Mar 24, 2026 at 7:58 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Tue, Mar 24, 2026 at 07:00:29PM +0530, AyushMukkanwar wrote:
> > > > > > > Add pr_fmt macro to prefix log messages with the module
> > > > > > > name for easier debugging.
> > > > > > >
> > > > > > > Signed-off-by: AyushMukkanwar <ayushmukkanwar@gmail.com>
> > > > > > > ---
> > > > > > >  drivers/staging/octeon/ethernet.c | 1 +
> > > > > > >  1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c
> > > > > > > index eadb74fc14c8..5bb8c303f88b 100644
> > > > > > > --- a/drivers/staging/octeon/ethernet.c
> > > > > > > +++ b/drivers/staging/octeon/ethernet.c
> > > > > > > @@ -5,6 +5,7 @@
> > > > > > >   * Copyright (c) 2003-2007 Cavium Networks
> > > > > > >   */
> > > > > > >
> > > > > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > > > >  #include <linux/platform_device.h>
> > > > > > >  #include <linux/kernel.h>
> > > > > > >  #include <linux/module.h>
> > > > > > > --
> > > > > > > 2.53.0
> > > > > > >
> > > > > >
> > > > > > How about working to remove the existing pr_*() calls with the proper
> > > > > > dev_*() and netdev_*() calls instead, so that pr_fmt() is not needed at
> > > > > > all?  That is the more "correct" solution here.
> > > > > >
> > > > > > thanks,
> > > > > >
> > > > > > greg k-h
> > > > >
> > > > > Hi Greg,
> > > > >
> > > > > After investigating, the pr_*() calls in ethernet-mem.c and
> > > > > ethernet-spi.c are inside functions that only receive hardware pool
> > > > > indices or register structs with no net_device or device pointer
> > > > > available, so dev_*() and netdev_*() replacements are not possible
> > > > > there.
> > > > >
> > > > > For ethernet.c, device pointers are available at most call sites and I
> > > > > can replace those with dev_err() and netdev_err()/netdev_info()
> > > > > appropriately. The two calls where no device pointer is available
> > > > > would keep pr_err() as is.
> > > >
> > > > That's a good start, but for the others, work back up the call chain to
> > > > properly pass in a device pointer so that these warning/error messages
> > > > can get printed out properly.  Drivers should not have any "generic"
> > > > messages like that, as it does not show what device actually created the
> > > > message.
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > >
> > > Hi Greg,
> > >
> > > After tracing the call chains, the pr_warn() calls are inside
> > > ethernet-mem.c. cvm_oct_mem_fill_fpa() which eventually reaches them
> > > is called from two places:
> > >
> > > 1. ethernet.c via cvm_oct_configure_common_hw(), which is called from
> > > cvm_oct_probe() - pdev is available here.
> > > 2. cvm_oct_rx_refill_pool() in ethernet-rx.h, which is called from
> > > cvm_oct_poll() in ethernet-rx.c - no device pointer is available there
> > > as it is called by the NAPI subsystem and oct_rx_group has no device
> > > pointer either.
> > >
> > > Since both call sites share cvm_oct_mem_fill_fpa(), adding a struct
> > > device * parameter would break the second call site. The FPA pool is
> > > shared hardware owned by the platform device, so storing &pdev->dev in
> > > a static global during probe and using that in the mem functions in
> > > ethernet-mem.c seems like the right approach. This avoids changing any
> > > function parameters. The same global could also be used for the
> > > pr_err() calls in ethernet-spi.c, where the interrupt handler has no
> > > device pointer available either. Would that be acceptable, or is there
> > > a preferred way to handle this?
> >
> > Drivers should NEVER have "shared hardware pools", they are always
> > device specific, so passing in the proper device everywhere is the
> > correct thing to be doing here.
> >
> > thanks,
> >
> > greg k-h
> 
> Hi Greg,
> 
> I have made the changes for most of the call chains. However there is
> one remaining blocker in ethernet.c.
> 
> cvm_oct_rx_refill_worker() in ethernet.c is a work queue callback with
> a fixed signature that only receives struct work_struct *work. It has
> no device pointer to pass to the cvm_oct_rx_refill_pool() call inside
> it.
> Currently cvm_oct_rx_refill_work is declared as a plain global
> delayed_work in ethernet.c with no parent struct, so container_of
> cannot be used to get a device pointer inside the worker.
> 
> One approach would be to embed the delayed_work inside a new struct
> that also holds a struct device * pointer, set it during probe, and
> use container_of inside the worker to get it. Is this the right
> approach, or is there a preferred approach?

There should not be any global data in a driver, it should all be on a
per-device basis, so if a workqueue needs to be added to the
device-specific structure, that's the correct solution to make.

thanks,

greg k-h