From: "Dale Farnsworth" <dale@farnsworth•org>
To: Arnd Bergmann <arnd@arndb•de>
Cc: linuxppc-dev@ozlabs•org
Subject: Re: [PATCH 10/13] powerpc: Add arch/powerpc mv64x60 PCI setup
Date: Thu, 3 May 2007 06:45:58 -0700 [thread overview]
Message-ID: <20070503134558.GC7491@xyzzy.farnsworth.org> (raw)
In-Reply-To: <200705030917.47037.arnd@arndb.de>
On Thu, May 03, 2007 at 09:17:46AM +0200, Arnd Bergmann wrote:
> On Wednesday 02 May 2007, Dale Farnsworth wrote:
>
> > Index: linux-2.6-powerpc-df/arch/powerpc/sysdev/Makefile
> > ===================================================================
> > --- linux-2.6-powerpc-df.orig/arch/powerpc/sysdev/Makefile
> > +++ linux-2.6-powerpc-df/arch/powerpc/sysdev/Makefile
> > @@ -16,6 +16,10 @@ obj-$(CONFIG_TSI108_BRIDGE) += tsi108_pc
> > obj-$(CONFIG_QUICC_ENGINE) += qe_lib/
> > obj-$(CONFIG_MV64X60) += mv64x60_pic.o mv64x60_dev.o
> >
> > +ifeq ($(CONFIG_PCI),y)
> > +obj-$(CONFIG_MV64X60) += mv64x60_pci.o
> > +endif
> > +
>
> I'd write this as
>
> mv64x60-$(CONFIG_INDIRECT_PCI) += mv64x60_pci.o
> obj-$(CONFIG_MV64X60) += mv64x60-y
>
> though that doesn't make much difference any more
>
> > +#ifdef CONFIG_SYSFS
> > +/* 32-bit hex or dec stringified number + '\n' */
> > +#define MV64X60_VAL_LEN_MAX 11
> > +#define MV64X60_PCICFG_CPCI_HOTSWAP 0x68
> > +
> > +DECLARE_MUTEX(mv64x60_hs_lock);
>
> Please avoid using struct semephores in new code, we now have struct mutex
> for this, which gets defined as
>
> static DEFINE_MUTEX(mv64x60_hs_mutex);
This is existing code being moved over from arch/ppc. I'll make the change.
> > +static ssize_t mv64x60_hs_reg_read(struct kobject *kobj, char *buf, loff_t off,
> > + size_t count)
> > +{
> > + u32 v;
> > + int save_exclude;
> > +
> > + if (off > 0)
> > + return 0;
> > + if (count < MV64X60_VAL_LEN_MAX)
> > + return -EINVAL;
> > +
> > + if (down_interruptible(&mv64x60_hs_lock))
> > + return -ERESTARTSYS;
> > + save_exclude = mv64x60_pci_exclude_bridge;
> > + mv64x60_pci_exclude_bridge = 0;
> > + early_read_config_dword(mv64x60_primary_hose, 0, PCI_DEVFN(0, 0),
> > + MV64X60_PCICFG_CPCI_HOTSWAP, &v);
>
> Why do you use early_read_config_dword, not pci_read_config_dword()?
As above, this is existing code being moved over from arch/ppc.
I'll make the change.
> > + mv64x60_pci_exclude_bridge = save_exclude;
> > + up(&mv64x60_hs_lock);
> > +
> > + return sprintf(buf, "0x%08x\n", v);
> > +}
>
> <snip>
>
> > +static int mv64x60_exclude_device(u_char bus, u_char devfn)
> > +{
> > + if ((bus == 0 || bus == mv64x60_pci2_busno) &&
> > + PCI_SLOT(devfn) == 0 && mv64x60_pci_exclude_bridge)
> > + return PCIBIOS_DEVICE_NOT_FOUND;
> > +
> > + return PCIBIOS_SUCCESSFUL;
> > +}
>
> The locking here looks wrong. If you call mv64x60_exclude_device() from one thread
> thread while another one is calling mv64x60_hs_reg_read(), the bridge will
> not be excluded.
Good catch. Again, it's existing code, but clearly there's a race here.
I'm guessing that the mutex above was just to prevent nesting of setting
mv64x60_pci_exclude_bridge. A fix for the race doesn't look easy.
I'll look into it.
Thanks,
-Dale
next prev parent reply other threads:[~2007-05-03 13:46 UTC|newest]
Thread overview: 87+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-04-25 23:46 [PATCH 0/13] powerpc: Add support for Marvell/mv64x60 and prpmc2800 Mark A. Greer
2007-04-25 23:55 ` [PATCH 1/13] powerpc: Add Makefile rule to wrap dts file in zImage Mark A. Greer
2007-04-30 6:06 ` David Gibson
2007-04-25 23:55 ` [PATCH 2/13] powerpc: Add dt_xlate_addr() to bootwrapper Mark A. Greer
2007-04-26 16:44 ` Scott Wood
2007-04-27 5:55 ` Paul Mackerras
2007-04-27 20:48 ` Mark A. Greer
2007-04-25 23:56 ` [PATCH 3/13] powerpc: Add bootwrapper support for Marvell/mv64x60 hostbridge Mark A. Greer
2007-04-27 6:01 ` Paul Mackerras
2007-04-27 22:02 ` Mark A. Greer
2007-05-03 5:25 ` Paul Mackerras
2007-05-03 18:44 ` Mark A. Greer
2007-05-03 19:00 ` Mark A. Greer
2007-05-05 23:27 ` Paul Mackerras
2007-05-07 18:15 ` Mark A. Greer
2007-04-25 23:57 ` [PATCH 4/13] powerpc: Add bootwrapper support for Marvell MPSC Mark A. Greer
2007-04-25 23:57 ` [PATCH 5/13] powerpc: Add bootwrapper support for Marvell/mv64x60 I2C Mark A. Greer
2007-04-25 23:58 ` [PATCH 6/13] powerpc: Add arch/powerpc support for Marvell/mv64x60 hostbridge Mark A. Greer
2007-04-26 0:42 ` Arnd Bergmann
2007-04-26 5:49 ` Dale Farnsworth
2007-05-02 21:38 ` [PATCH 6/13] powerpc: Add arch/powerpc interrupt handler for mv64x60 Dale Farnsworth
2007-05-03 1:47 ` Stephen Rothwell
2007-05-03 2:55 ` Dale Farnsworth
2007-04-25 23:59 ` [PATCH 7/13] powerpc: Add arch/powerpc mv64x60 MPSC platform data setup Mark A. Greer
2007-04-26 0:14 ` Arnd Bergmann
2007-04-26 5:57 ` Dale Farnsworth
2007-04-26 11:24 ` Arnd Bergmann
2007-04-26 14:30 ` Dale Farnsworth
2007-04-26 15:14 ` Arnd Bergmann
2007-05-02 21:41 ` Dale Farnsworth
2007-05-03 6:40 ` Arnd Bergmann
2007-05-04 21:03 ` [PATCH 7/13] powerpc: Create Marvell mv64x60 MPSC (serial) platform_data Dale Farnsworth
2007-05-05 12:26 ` Arnd Bergmann
2007-04-26 0:00 ` [PATCH 8/13] powerpc: Add arch/powerpc mv64x60_eth platform data setup Mark A. Greer
2007-04-26 0:18 ` Arnd Bergmann
2007-04-26 6:00 ` Dale Farnsworth
2007-05-02 21:43 ` Dale Farnsworth
2007-05-03 2:03 ` Stephen Rothwell
2007-05-03 6:43 ` Arnd Bergmann
2007-05-04 21:06 ` [PATCH 8/13] powerpc: Create Marvell mv64x60 ethernet platform_data Dale Farnsworth
2007-05-05 12:28 ` Arnd Bergmann
2007-04-26 0:00 ` [PATCH 9/13] powerpc: Add arch/powerpc mv64x60 I2C platform data setup Mark A. Greer
2007-04-26 0:21 ` Arnd Bergmann
2007-04-26 0:43 ` Mark A. Greer
2007-04-26 0:55 ` Arnd Bergmann
2007-04-26 1:13 ` Mark A. Greer
2007-04-26 2:02 ` Arnd Bergmann
2007-04-26 6:08 ` Dale Farnsworth
2007-04-26 9:00 ` Arnd Bergmann
2007-04-26 14:19 ` Dale Farnsworth
2007-04-26 15:04 ` Arnd Bergmann
2007-04-27 23:50 ` Dale Farnsworth
2007-04-28 1:05 ` Arnd Bergmann
2007-04-28 2:40 ` Dale Farnsworth
2007-05-01 4:58 ` Paul Mackerras
2007-05-01 4:45 ` Paul Mackerras
2007-04-26 6:48 ` Mark A. Greer
2007-05-02 21:44 ` Dale Farnsworth
2007-05-03 6:53 ` Arnd Bergmann
2007-05-03 13:06 `
2007-05-04 21:08 ` [PATCH 9/13] powerpc: Create Marvell mv64x60 I2C platform_data Dale Farnsworth
2007-05-05 12:29 ` Arnd Bergmann
2007-04-26 0:01 ` [PATCH 10/13] powerpc: Add arch/powerpc mv64x60 PCI setup Mark A. Greer
2007-04-26 0:25 ` Arnd Bergmann
2007-04-26 6:33 ` Dale Farnsworth
2007-04-26 11:39 ` Arnd Bergmann
2007-04-26 14:42 ` Dale Farnsworth
2007-05-02 21:46 ` Dale Farnsworth
2007-05-03 2:13 ` Stephen Rothwell
2007-05-03 2:57 ` Dale Farnsworth
2007-05-03 7:17 ` Arnd Bergmann
2007-05-03 13:45 ` Dale Farnsworth [this message]
2007-05-04 21:10 ` [PATCH 10/13] powerpc: Add Marvell mv64x60 PCI bridge support Dale Farnsworth
2007-05-05 12:30 ` Arnd Bergmann
2007-04-26 0:01 ` [PATCH 11/13] powerpc: Add DTS file for the Motorola PrPMC2800 platform Mark A. Greer
2007-04-26 16:42 ` Scott Wood
2007-04-26 23:34 ` Mark A. Greer
2007-04-26 23:37 ` David Gibson
2007-04-27 20:41 ` Mark A. Greer
2007-04-30 16:45 ` Jon Loeliger
2007-04-30 18:08 ` Mark A. Greer
2007-04-30 22:29 ` Mark A. Greer
2007-04-26 0:02 ` [PATCH 12/13] powerpc: Add bootwrapper support for " Mark A. Greer
2007-04-26 0:02 ` [PATCH 13/13] powerpc: Add arch/powerpc support for the " Mark A. Greer
2007-04-26 0:45 ` [PATCH 0/13] powerpc: Add support for Marvell/mv64x60 and prpmc2800 David Gibson
2007-04-26 0:58 ` Mark A. Greer
2007-04-26 1:15 ` Mark A. Greer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20070503134558.GC7491@xyzzy.farnsworth.org \
--to=dale@farnsworth$(echo .)org \
--cc=arnd@arndb$(echo .)de \
--cc=linuxppc-dev@ozlabs$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox