netdev - Re: [PATCH net-next v2 2/4] can: cc770: add legacy ISA bus driver for the CC770 and AN82527

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4F0D56EC.5080704@pengutronix.de>
Date:	Wed, 11 Jan 2012 10:31:24 +0100
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	info@...ax.com
CC:	Wolfgang Grandegger <wg@...ndegger.com>,
	David Laight <David.Laight@...LAB.COM>,
	Oliver Hartkopp <socketcan@...tkopp.net>, henrik@...conx.com,
	netdev@...r.kernel.org, linux-can@...r.kernel.org,
	socketcan-users@...ts.berlios.de, IreneV <boir1@...dex.ru>,
	Stanislav Yelenskiy <stanislavelensky@...oo.com>, oe@...t.de,
	henrik@...us-sw.com
Subject: Re: [PATCH net-next v2 2/4] can: cc770: add legacy ISA bus driver
 for the CC770 and AN82527

On 01/11/2012 10:05 AM, Wolfgang Zarre wrote:
[...]

The patch looks good, it has some style issues though, see comments
inline. Please fix them, add a description to the patch, don't forget
your S-o-b, same applies to the other patch which removes the extra IRQ
ack. Please create a series of two patches, first this (the spinlock
patch), the second the rermoval of the extra IRQ ack. Send this patches
to this list. See [1] for more information about patch submitting.

Marc

[1] http://lxr.linux.no/linux+v3.2/Documentation/SubmittingPatches#L86

> Test was successful, 2.000.000 telegrams again without problems.
> Here so far the patch as You suggested:
> 
> -------------------------------------------------------------------
>  diff --git a/drivers/net/can/cc770/cc770_isa.c
> b/drivers/net/can/cc770/cc770_isa.c
> index 4be5fe2..adf3e45 100644
> --- a/drivers/net/can/cc770/cc770_isa.c
> +++ b/drivers/net/can/cc770/cc770_isa.c
> @@ -110,6 +110,10 @@ MODULE_PARM_DESC(bcr, "Bus configuration register
> (default=0x40 [CBY])");
>  #define CC770_IOSIZE          0x20
>  #define CC770_IOSIZE_INDIRECT 0x02
> 
> +/* Spinlock for cc770_isa_port_write_reg_indirect */

The spinlock is used in read_reg_indirect, too

> +static DEFINE_SPINLOCK(cc770_isa_port_lock);
> +
> +

Just one new blank line please.

>  static struct platform_device *cc770_isa_devs[MAXDEV];
> 
>  static u8 cc770_isa_mem_read_reg(const struct cc770_priv *priv, int reg)
> @@ -138,18 +142,26 @@ static u8 cc770_isa_port_read_reg_indirect(const
> struct cc770_priv *priv,
>                           int reg)
>  {
>      unsigned long base = (unsigned long)priv->reg_base;
> +    unsigned long flags;
> +    u8 val;
> 
> +    spin_lock_irqsave(&cc770_isa_port_lock, flags);
>      outb(reg, base);
> -    return inb(base + 1);
> +    val = inb(base + 1);
> +    spin_unlock_irqrestore(&cc770_isa_port_lock, flags);

Please insert a newline before return.

> +    return val;
>  }
> 
>  static void cc770_isa_port_write_reg_indirect(const struct cc770_priv
> *priv,
>                          int reg, u8 val)
>  {
>      unsigned long base = (unsigned long)priv->reg_base;
> +    unsigned long flags;
> 
> +    spin_lock_irqsave(&cc770_isa_port_lock, flags);
>      outb(reg, base);
>      outb(val, base + 1);
> +    spin_unlock_irqrestore(&cc770_isa_port_lock, flags);    
>  }
> 
>  static int __devinit cc770_isa_probe(struct platform_device *pdev)
> -------------------------------------------------------


-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


Download attachment "signature.asc" of type "application/pgp-signature" (263 bytes)