lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 1 Sep 2014 14:46:10 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:	Bjorn Helgaas <bhelgaas@...gle.com>, linux-kernel@...r.kernel.org,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Chang Rebecca Swee Fun <rebecca.swee.fun.chang@...el.com>
Subject: Re: [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code

On Mon, 01 Sep 2014, Andy Shevchenko wrote:

> On Mon, 2014-09-01 at 12:38 +0100, Lee Jones wrote:
> > On Mon, 01 Sep 2014, Andy Shevchenko wrote:
> > 
> > > On Mon, 2014-09-01 at 10:13 +0100, Lee Jones wrote:
> > > > On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> > > > 
> > > > > This patch refactors the driver to use helper functions instead of
> > > > > copy'n'pasted pieces of code.
> > > > > 
> > > > > Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@...el.com>
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > > > > ---
> > > > >  drivers/mfd/lpc_sch.c | 146 ++++++++++++++++++++++----------------------------
> > > > >  1 file changed, 64 insertions(+), 82 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> > > > > index 4ee7550..0f01ef0 100644
> > > > > --- a/drivers/mfd/lpc_sch.c
> > > > > +++ b/drivers/mfd/lpc_sch.c
> > > > > @@ -40,41 +40,6 @@
> > > > 
> > > > [...]
> > > 
> > > Thanks for review, my answers below.
> > > 
> > > > 
> > > > > -static int lpc_sch_probe(struct pci_dev *dev,
> > > > > -				const struct pci_device_id *id)
> > > > > +static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
> > > > > +			  struct resource *res, int size)
> > > > >  {
> > > > >  	unsigned int base_addr_cfg;
> > > > >  	unsigned short base_addr;
> > > > > -	int i, cells = 0;
> > > > > -	int ret;
> > > > >  
> > > > > -	pci_read_config_dword(dev, SMBASE, &base_addr_cfg);
> > > > > +	pci_read_config_dword(pdev, where, &base_addr_cfg);
> > > > >  	base_addr = 0;
> > > > >  	if (!(base_addr_cfg & (1 << 31)))
> > > > > -		dev_warn(&dev->dev, "Decode of the SMBus I/O range disabled\n");
> > > > > +		dev_warn(&pdev->dev, "Decode of the %s I/O range disabled\n",
> > > > > +			 name);
> > > > >  	else
> > > > >  		base_addr = (unsigned short)base_addr_cfg;
> > > > >  
> > > > >  	if (base_addr == 0) {
> > > > > -		dev_warn(&dev->dev, "I/O space for SMBus uninitialized\n");
> > > > > -	} else {
> > > > > -		lpc_sch_cells[cells++] = isch_smbus_cell;
> > > > > -		smbus_sch_resource.start = base_addr;
> > > > > -		smbus_sch_resource.end = base_addr + SMBUS_IO_SIZE - 1;
> > > > > +		dev_warn(&pdev->dev, "I/O space for %s uninitialized\n", name);
> > > > > +		return -ENODEV;
> > > > 
> > > > If you're going to return an error, you need to use dev_err() above.
> > > 
> > > Okay.
> > > 
> > > > >  	}
> > > > >  
> > > > > -	pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg);
> > > > > -	base_addr = 0;
> > > > > -	if (!(base_addr_cfg & (1 << 31)))
> > > > > -		dev_warn(&dev->dev, "Decode of the GPIO I/O range disabled\n");
> > > > > +	res->start = base_addr;
> > > > > +	res->end = base_addr + size - 1;
> > > > > +	res->flags = IORESOURCE_IO;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> > > > > +				 const char *name, int size, int id,
> > > > > +				 struct mfd_cell *cell)
> > > > > +{
> > > > > +	struct resource *res;
> > > > > +	int ret;
> > > > > +
> > > > > +	res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL);
> > > > > +	if (!res)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	ret = lpc_sch_get_io(pdev, where, name, res, size);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	memset(cell, 0, sizeof(*cell));
> > > > > +
> > > > > +	cell->name = name;
> > > > > +	cell->resources = res;
> > > > > +	cell->num_resources = 1;
> > > > > +	cell->ignore_resource_conflicts = true;
> > > > > +	cell->id = id;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int lpc_sch_probe(struct pci_dev *dev,
> > > > > +			 const struct pci_device_id *id)
> > > > > +{
> > > > > +	struct mfd_cell lpc_sch_cells[3];
> > > > > +	int size, cells = 0;
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
> > > > > +				    id->device, &lpc_sch_cells[cells]);
> > > > > +	if (!ret)
> > > > > +		cells++;
> > > > 
> > > > You're masking errors here.  You need to return on error.
> > > 
> > > No, I don't. It's a local error, I just need to understand if the HW has
> > > or hasn't the IP part we're looking for.
> > 
> > Yes, you do.  -ENOMEM is not a local error, it needs to be returned.
> > 
> > > I could, let's say, return true or false, if you prefer, with the above
> > > meaning.
> > 
> > I don't see this very often, which makes me wonder if it's the right
> > thing to do at all; however, if you're going to this then please
> > return a local state, instead of a Linux Error Code.
> 
> Okay, what about 1 — IP is present and we have no error, 0 — no ip, no
> error, -err — error that should be returned, would it work for you?

I would suggest -E<error_message> for messages to be passed back, 0
for 'everything found and correct' and >0 (#defined of course) for any
local messages you wish to pass 'IP not found', etc.

> > #define LCP_SKIP_RESOURCE /* Seems resonable? */
> 
> 
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists