Re: [PATCH v2 1/2] staging: octeon: ethernet-spi: replace pr_err with dev_err

[Greg KH]

On Sat, Apr 04, 2026 at 02:50:56PM +0530, Ayush Mukkanwar wrote:
> On Wed, Apr 1, 2026 at 3:39 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Tue, Mar 31, 2026 at 04:47:56PM +0530, AyushMukkanwar wrote:
> > > Replace pr_err() calls with dev_err() to include device information
> > > in log messages. The device pointer is passed through the interrupt
> > > handler via dev_id, which is changed from &number_spi_ports to
> > > dev->dev.parent in request_irq and free_irq.
> > >
> > > Signed-off-by: AyushMukkanwar <ayushmukkanwar@xxxxxxxxx>
> > > ---
> > >  drivers/staging/octeon/ethernet-spi.c | 59 ++++++++++++++-------------
> > >  1 file changed, 30 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/drivers/staging/octeon/ethernet-spi.c b/drivers/staging/octeon/ethernet-spi.c
> > > index 699c98c5ec13..8c02920c3cdc 100644
> > > --- a/drivers/staging/octeon/ethernet-spi.c
> > > +++ b/drivers/staging/octeon/ethernet-spi.c
> > > @@ -17,67 +17,67 @@
> > >  static int number_spi_ports;
> > >  static int need_retrain[2] = { 0, 0 };
> > >
> > > -static void cvm_oct_spxx_int_pr(union cvmx_spxx_int_reg spx_int_reg, int index)
> > > +static void cvm_oct_spxx_int_pr(union cvmx_spxx_int_reg spx_int_reg, int index, struct device *dev)
> >
> > This is ok, but this, but usually the pointer is the first argument.
> > And shouldn't this be a netdev (see below...)
> >
> > > @@ -107,14 +107,15 @@ static irqreturn_t cvm_oct_spi_rml_interrupt(int cpl, void *dev_id)
> > >  {
> > >       irqreturn_t return_status = IRQ_NONE;
> > >       union cvmx_npi_rsl_int_blocks rsl_int_blocks;
> > > +     struct device *dev = dev_id;
> >
> > This isn't ok, the function prototype should really be a pointer, not a
> > void thing.
> >
> > > @@ -196,7 +197,7 @@ int cvm_oct_spi_init(struct net_device *dev)
> > >
> > >       if (number_spi_ports == 0) {
> > >               r = request_irq(OCTEON_IRQ_RML, cvm_oct_spi_rml_interrupt,
> > > -                             IRQF_SHARED, "SPI", &number_spi_ports);
> > > +                             IRQF_SHARED, "SPI", dev->dev.parent);
> >
> > Wait, no, you can't do anything with the parent!
> >
> > This is a netdev, keep it a netdev.  Don't pass it "up" the device
> > heirchary to the device pointer, use the real netdev pointer instead.
> >
> > Also, it seems you didn't test build your changes, what happened?
> >
> > thanks,
> >
> > greg k-h
> 
> Hi Greg,
> 
> I'm preparing v4 of the octeon pr_*() conversion series, broken into
> smaller patches as you requested.
> 
> While converting ethernet-spi.c, I hit a complication with the IRQ
> handler. The SPI code registers a single shared IRQ across all SPI
> ports:
> if (number_spi_ports == 0)
>     request_irq(OCTEON_IRQ_RML, handler, IRQF_SHARED, "SPI", dev_id);
> number_spi_ports++;
> 
> The original code used &number_spi_ports (a static) as dev_id, so
> request_irq and free_irq always matched. But to convert the handler's
> pr_err() calls to netdev_err(), I need to pass a net_device * as
> dev_id.
> 
> The problem is: the first SPI port registers the IRQ, but the last
> port to uninit frees it with a different net_device *, causing a
> dev_id mismatch.
> 
> Additionally, all the error messages in the handler are SPI4 bus-level
> errors (DIP4, calendar parity, FIFO overflow, etc.), not specific to
> any individual port. Would using dev_err() with the platform device be
> acceptable here since the IRQ is registered once for the whole SPI
> subsystem? Or would you prefer a different approach?

Use your best judgement here, it sounds like this driver needs lots of
cleanups!

good luck,

greg k-h