Re: [PATCH 1/1] edac: Add an API for edac device to report for multiple errors

[Robert Richter <rrichter@marvell.com>]

On 08.09.19 10:58:31, Hawa, Hanna wrote:
> On 9/5/2019 12:56 PM, Robert Richter wrote:

> > > diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c
> > > index 65cf2b9355c4..bf6a4fd9831b 100644
> > > --- a/drivers/edac/edac_device.c
> > > +++ b/drivers/edac/edac_device.c
> > > @@ -555,12 +555,15 @@ static inline int edac_device_get_panic_on_ue(struct edac_device_ctl_info
> > >   	return edac_dev->panic_on_ue;
> > >   }
> > > -void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
> > > -			int inst_nr, int block_nr, const char *msg)
> > > +static void __edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
> > > +			   u16 error_count, int inst_nr, int block_nr,
> > 
> > Just curious, why u16, some register mask size? Maybe just use unsigned int?
> 
> Wanted to be aligned with edac MC.
> I can change it to be u32.

There is no specific reason for u32 either. This code is generic for
many machines and compilers, so unsigned int seems to be the best
choice here to get an optimum.

> > I think the variable can be shortened to 'count', the meaning should
> > still be clear.
> 
> I think more clear to include 'error'.
> maybe shorter name 'err_count'?

IMO 'count' is clear here, 'error' isn't. I prefer short names for
local variables and arguments.

> > > +void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
> > > +			   int inst_nr, int block_nr, const char *msg)
> > > +{
> > > +	__edac_device_handle_ce(edac_dev, 1, inst_nr, block_nr, msg);
> > >   }
> > >   EXPORT_SYMBOL_GPL(edac_device_handle_ce);
> > 
> > We could just export the __*() version of those functions and make
> > everything else inline in the header file? Though, better do this with
> > two patches to avoid an ABI breakage in case someone wants to backport
> > it. Let's see what others say here.
> 
> Waiting for other reviewers.

If no one else complains I would prefer moving to
__edac_device_handle_* as exported sympbol. But make this change in 2
patches to make backports easy, first add __edac_device_handle_*()
(and introduce the *_count() inline functions), then remove
edac_device_handle_*() entirely.

Thanks,

-Robert