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 linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 27 Jul 2007 12:38:53 -0700 From: Andrew Morton <akpm@...ux-foundation.org> To: Michael Buesch <mb@...sch.de> Cc: "linux-kernel" <linux-kernel@...r.kernel.org>, bcm43xx-dev@...ts.berlios.de, netdev@...r.kernel.org, linux-wireless@...r.kernel.org, Gary Zambrano <zambrano@...adcom.com> Subject: Re: [PATCH] Merge the Sonics Silicon Backplane subsystem On Fri, 27 Jul 2007 21:30:48 +0200 Michael Buesch <mb@...sch.de> wrote: > > ERROR: "foo * bar" should be "foo *bar" > > #4156: FILE: drivers/ssb/ssb_private.h:119: > > +extern struct ssb_bus * ssb_pci_dev_to_bus(struct pci_dev *pdev); > > > > are worth addressing. > > Well, I intentionally wrote that this way, as in my opinion > it it easier to read. I only use this additional space for > functions returning a pointer. > > struct foo * function(int a, int b); > > vs: > > struct foo *function(int a, int b); > > But I can change that, if that's really an issue and a > style violation. It's a microissue but yeah, no-space is more conventional. > > > +static ssize_t ssb_pci_attr_sprom_show(struct device *pcidev, > > > + struct device_attribute *attr, > > > + char *buf) > > > +{ > > > + struct pci_dev *pdev = container_of(pcidev, struct pci_dev, dev); > > > + struct ssb_bus *bus; > > > + u16 *sprom; > > > + int err = -ENODEV; > > > + ssize_t count = 0; > > > + > > > + bus = ssb_pci_dev_to_bus(pdev); > > > + if (!bus) > > > + goto out; > > > + err = -ENOMEM; > > > + sprom = kcalloc(SSB_SPROMSIZE_WORDS, sizeof(u16), GFP_KERNEL); > > > + if (!sprom) > > > + goto out; > > > + > > > + err = -ERESTARTSYS; > > > + if (mutex_lock_interruptible(&bus->pci_sprom_mutex)) > > > + goto out_kfree; > > > + sprom_do_read(bus, sprom); > > > + mutex_unlock(&bus->pci_sprom_mutex); > > > + > > > + count = sprom2hex(sprom, buf, PAGE_SIZE); > > > + err = 0; > > > + > > > +out_kfree: > > > + kfree(sprom); > > > +out: > > > + return err ? err : count; > > > +} > > > > The mutex_lock_interruptible() looks fishy. Some commented explanation of > > what it's doing would be good here. It's quite unobvious to this reader. > > Cheesy deadlock avoidance? Hope not. > > No, it's simply to avoid writing the SPROM concurrently. > SPROM writing is hairy and we must make sure here that > we are the only one accessing the whole bus. We do that > by suspending all devices and taking a lock to protect > the SPROM from write concurrency. Sure, but why is the locking interruptible rather than plain old mutex_lock()? - 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