| 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
| ||
|
Message-Id: <20061209141428.2ef3bfb5.akpm@osdl.org>
Date: Sat, 9 Dec 2006 14:14:28 -0800
From: Andrew Morton <akpm@...l.org>
To: Tony Olech <tony.olech@...ndigitalsystems.com>
Cc: Greg KH <greg@...ah.com>,
USB Development List <linux-usb-devel@...ts.sourceforge.net>,
Kernel Development List <linux-kernel@...r.kernel.org>,
Bart Prescott <bart.prescott@...ndigitalsystems.com>
Subject: Re: [PATCH] USB: u132-hcd/ftdi-elan: add support for Option GT 3G
Quad card
On Wed, 06 Dec 2006 13:16:22 +0000
Tony Olech <tony.olech@...ndigitalsystems.com> wrote:
> From: Tony Olech <tony.olech@...ndigitalsystems.com>
>
> ELAN's U132 is a USB to CardBus OHCI controller adapter,
> designed specifically for CardBus 3G data cards to
> function in machines without a CardBus slot.
> The "ftdi-elan" module is a USB client driver, that detects
> a supported CardBus OHCI controller plugged into the
> U132 adapter and thereafter provides the conduit for
> for access by the "u132-hcd" module.
> The "u132-hcd" module is a (cut-down OHCI) host controller
> that supports a single OHCI function of the CardBus
> card inserted into the U132 adapter.
>
> ...
>
Several coding style issues here.
> struct usb_hcd *hcd;
> + int retval;
> + u32 control;
> + u32 rh_a = -1;
> + u32 num_ports;
> msleep(100);
We usually put a blank line between the end of the auto definitions and the
start of the code.
> if (u132_exiting > 0) {
> return -ENODEV;
> - } /* refuse to confuse usbcore */
> + }
Unneeded braces.
> + retval = ftdi_write_pcimem(pdev, intrdisable, OHCI_INTR_MIE);
THis entire patch uses spaces where it should be using tabs!
> + if (hc_fminterval != FI) {
> + }
?
> + if (retval)
> + return retval;
> + retry:retval = ftdi_read_pcimem(ftdi, cmdstatus, &status);
> + if (retval)
> + return retval;
No, labels are done line this:
if (retval)
return retval;
retry:
retval = ftdi_read_pcimem(ftdi, cmdstatus, &status);
if (retval)
return retval;
(the driver appears to already use the wrong style - just fix it up as you
go).
> + retval = ftdi_write_pcimem(ftdi, cmdstatus, OHCI_HCR);
> + if (retval)
> + return retval;
> + extra:{
> + retval = ftdi_read_pcimem(ftdi, cmdstatus, &status);
> + if (retval)
> + return retval;
> + if (0 != (status & OHCI_HCR)) {
> + if (--reset_timeout == 0) {
> + dev_err(&ftdi->udev->dev, "USB HC reset timed o"
> + "ut!\n");
> + return -ENODEV;
> + } else {
> + msleep(5);
> + goto extra;
> + }
> + }
> + }
The extra set of braces around this block is unnecessary. Remove that,
save a tbstop, then un-break that string.
> + if (0 == (fminterval & 0x3fff0000) || 0 == periodicstart) {
This trick makes people's brains explode. Please just do
if (foo == 0)
> + mdelay((roothub_a >> 23) & 0x1fe);
mdelay is a busy-wait. It needs super-special jsutification. Certainly a
comment explaining why it is really necessary, and perhaps some soothing
words about the expected upper-bound.
> +static int ftdi_elan_enumeratePCI(struct usb_ftdi *ftdi)
> +{
> + u32 controlreg;
> + u8 sensebits;
> + int UxxxStatus;
> + UxxxStatus = ftdi_elan_read_reg(ftdi, &controlreg);
> + if (UxxxStatus)
> + return UxxxStatus;
> + UxxxStatus = ftdi_elan_write_reg(ftdi, 0x00000000L);
> + if (UxxxStatus)
> + return UxxxStatus;
> + msleep(750);
> + UxxxStatus = ftdi_elan_write_reg(ftdi, 0x00000200L | 0x100);
> + if (UxxxStatus)
> + return UxxxStatus;
> + UxxxStatus = ftdi_elan_write_reg(ftdi, 0x00000200L | 0x500);
> + if (UxxxStatus)
> + return UxxxStatus;
> + UxxxStatus = ftdi_elan_read_reg(ftdi, &controlreg);
> + if (UxxxStatus)
> + return UxxxStatus;
> + UxxxStatus = ftdi_elan_write_reg(ftdi, 0x0000020CL | 0x000);
> + if (UxxxStatus)
> + return UxxxStatus;
> + UxxxStatus = ftdi_elan_write_reg(ftdi, 0x0000020DL | 0x000);
> + if (UxxxStatus)
> + return UxxxStatus;
> + msleep(250);
> + UxxxStatus = ftdi_elan_write_reg(ftdi, 0x0000020FL | 0x000);
> + if (UxxxStatus)
> + return UxxxStatus;
> + UxxxStatus = ftdi_elan_read_reg(ftdi, &controlreg);
> + if (UxxxStatus)
> + return UxxxStatus;
> + UxxxStatus = ftdi_elan_write_reg(ftdi, 0x0000025FL | 0x800);
> + if (UxxxStatus)
> + return UxxxStatus;
> + UxxxStatus = ftdi_elan_read_reg(ftdi, &controlreg);
> + if (UxxxStatus)
> + return UxxxStatus;
> + UxxxStatus = ftdi_elan_read_reg(ftdi, &controlreg);
> + if (UxxxStatus)
> + return UxxxStatus;
> + msleep(1000);
> + sensebits = (controlreg >> 16) & 0x000F;
> + if (0x0D == sensebits)
> + return 0;
> + else
> + return - ENXIO;
> +}
Well it's certainly rigorous about error-checking.
I suspect a lot of this could become table-driven.
-
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