Re: [PATCH 2/3] mcb: Add PCI carrier for MEN Chameleon Bus

[Johannes Thumshirn <johannes.thumshirn@men.de>]

On Tue, Feb 18, 2014 at 05:20:17PM -0600, Josh Cartwright wrote:
> On Tue, Feb 18, 2014 at 04:34:13PM +0100, Johannes Thumshirn wrote:
> > Add support for MCB over PCI devices. Both PCI attached on-board Chameleon FPGAs
> > as well as CompactPCI based MCB carrier cards are supported with this driver.
> >
> > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@men.de>
> > ---
> >  drivers/mcb/Kconfig   |  13 ++++++
> >  drivers/mcb/Makefile  |   2 +
> >  drivers/mcb/mcb-pci.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 123 insertions(+)
> >  create mode 100644 drivers/mcb/mcb-pci.c
> >
> > diff --git a/drivers/mcb/Kconfig b/drivers/mcb/Kconfig
> > index 9e7d6f5..8b058bc 100644
> > --- a/drivers/mcb/Kconfig
> > +++ b/drivers/mcb/Kconfig
> > @@ -14,3 +14,16 @@ menuconfig MCB
> >
> >  	   If build as a module, the module is called mcb.ko
> >
> > +if MCB
> > +config MCB_PCI
> > +	   tristate "PCI based MCB carrier"
> > +	   default m if MCB
>
> 'if MCB' is redundant (MCB has to be set for this option to even be
> available).

OK

>
> [..]
> > +++ b/drivers/mcb/mcb-pci.c
> > @@ -0,0 +1,108 @@
>
> Copyright/license blurb?

Args, forgotten, sorry.

>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/mcb.h>
> > +
> > +#include "mcb-internal.h"
> > +
> > +static struct mcb_bus *bus;
> > +struct priv {
> > +	void __iomem *base;
> > +};
>
> This seems weird.  Why is the bus not part of your private data?  Seems
> like you're unnecessarily restricting yourself to supporting only one of
> these devices at a time...

You're right. Funny enough I didn't notice it, while testing as I used 2 identical
carrier cards. Card 2 must then have overwritten Card 1 :-(.

>
> > +
> > +static int mcb_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > +{
> > +	struct priv *priv;
> > +	phys_addr_t mapbase;
> > +	int ret = 0;
>
> No need to initialize.

OK

>
> > +	int num_cells;
> > +	unsigned long flags;
> > +
> > +	priv = devm_kzalloc(&pdev->dev, sizeof(struct priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	ret = pci_enable_device(pdev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to enable PCI device\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	mapbase = pci_resource_start(pdev, 0);
> > +	if (!mapbase) {
> > +		dev_err(&pdev->dev, "No PCI resource\n");
> > +		goto err_start;
> > +	}
> > +
> > +	ret = pci_request_region(pdev, 0, KBUILD_MODNAME);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to request PCI BARs\n");
> > +		goto err_start;
> > +	}
> > +
> > +	priv->base = pci_iomap(pdev, 0, 0);
>
> Hmm.  There should really be a devm_* variant for PCI resources.

I don't think so, as I have to free the resources afterwards again so the mcb
devices can access these resources. Otherwise I would need to make them muxed,
but I don't see the reason to keep the resources after the parser code has
finished.

>
> [..]
> > +
> > +static void mcb_pci_remove(struct pci_dev *pdev)
> > +{
> > +	struct priv *priv = pci_get_drvdata(pdev);
> > +
> > +	mcb_release_bus(bus);
> > +
> > +	pci_iounmap(pdev, priv->base);
> > +	pci_release_region(pdev, 0);
> > +	pci_disable_device(pdev);
> > +	pci_set_drvdata(pdev, NULL);
> > +}
> > +

It's actually a "double free" of the resources, isn't it. Needs to be fixed.

> > +static struct pci_device_id mcb_pci_tbl[] = {
>
> const?
>

Yup.

> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation

As with the other patch, probably on monday. Sorry for the inconvenience.

Johannes