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: Tue, 10 Jan 2012 15:25:17 +0100 From: Wolfgang Grandegger <wg@...ndegger.com> To: info@...ax.com CC: Marc Kleine-Budde <mkl@...gutronix.de>, 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/10/2012 03:20 PM, Wolfgang Grandegger wrote: > On 01/10/2012 01:30 PM, Wolfgang Zarre wrote: >> Hello Wolfgang, >> >>> On 01/10/2012 12:11 AM, Marc Kleine-Budde wrote: >>>> On 01/09/2012 10:47 PM, Wolfgang Zarre wrote: >>>> [...] >>>> >>>>>>> OK. My concern: Can we be sure that 16bit accesses are always >>>>>> supported >>>>>>> by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around >>>>>> the >>>>>>> 8bit accesses already help? >>>>>> >>>>>> Hmmm... are there any register reads that need the >>>>>> same 'double cycle' sequence ?? >>>>>> If so you need to stop reads being interleaved (with >>>>>> themselves and writes) so requesting a 16bit access >>>>>> doesn't help. >>>>>> >>>>>> Which means you need a spinlock... >>>>>> >>>>>> David >>>>>> >>>>>> >>>>> >>>>> @David: Thank You very much for that hint. You are right and to >>>>> implement correct we need a spinlock. >>>>> >>>>> @Wolfgang: I was thinking about Your question regarding 8/16 bit and >>>>> in fact it wouldn't work at all on a clean 8 bit cards. >>>>> >>>>> Further it wouldn't work on 16 bit cards where the MSB is not equal >>>>> to base port +1 and anyway, it's depending always on how the chip is >>>>> interfaced to the ISA bus and in which mode the chip is configured. >>>>> >>>>> >>>>> And therefore I was giving David's hint a try in using a spinlock in >>>>> function cc770_isa_port_write_reg_indirect() and patched as follows: >>>>> >>>>> --------------------------------------------------------------------- >>>>> diff --git a/drivers/net/can/cc770/cc770.c >>>>> b/drivers/net/can/cc770/cc770.c >>>>> index 2d12f89..dad6707 100644 >>>>> --- a/drivers/net/can/cc770/cc770.c >>>>> +++ b/drivers/net/can/cc770/cc770.c >>>>> @@ -460,15 +460,6 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff >>>>> *skb, struct net_device *dev) >>>>> >>>>> stats->tx_bytes += dlc; >>>>> >>>>> - >>>>> - /* >>>>> - * HM: We had some cases of repeated IRQs so make sure the >>>>> - * INT is acknowledged I know it's already further up, but >>>>> - * doing again fixed the issue >>>>> - */ >>>>> - cc770_write_reg(priv, msgobj[mo].ctrl0, >>>>> - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); >>>>> - >>>>> return NETDEV_TX_OK; >>>>> } >>>>> >>>>> @@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct net_device >>>>> *dev, unsigned int o) >>>>> /* Nothing more to send, switch off interrupts */ >>>>> cc770_write_reg(priv, msgobj[mo].ctrl0, >>>>> MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES); >>>>> - /* >>>>> - * We had some cases of repeated IRQ so make sure the >>>>> - * INT is acknowledged >>>>> - */ >>>>> - cc770_write_reg(priv, msgobj[mo].ctrl0, >>>>> - MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES); >>> >>> Please provide an extra patch for these unrelated changes. If we really >>> want to remove it. >>> >> >> Sure, this I can do. >> >>>>> stats->tx_packets++; >>>>> can_get_echo_skb(dev, 0); >>>>> diff --git a/drivers/net/can/cc770/cc770_isa.c >>>>> b/drivers/net/can/cc770/cc770_isa.c >>>>> index 4be5fe2..fe39eed 100644 >>>>> --- a/drivers/net/can/cc770/cc770_isa.c >>>>> +++ b/drivers/net/can/cc770/cc770_isa.c >>>>> @@ -110,6 +110,9 @@ 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 */ >>>>> +static DEFINE_SPINLOCK( outb_lock); > > Please use a more specific name, e.g.: cc770_isa_port_lock > >>>>> + >>>> >>>> Do we need a global or a per device spin lock? If this should be a per >>>> device one, please introduce a cc770_isa_priv and put the spinlock >>>> there. Don't forget to initialize the spinlock. >>> >>> Yes, that's what I was thinking as well but in the ocan driver I find: >>> >>> /* >>> * we need a spinlock here, as the address register looks shared between >>> * two PC-ECAN devices. Moreover, we need to protect WRT interrupts >>> */ >>> >>> Looks like wired hardware. Anyway, a global spinlock might be safer. >>> >> >> Hmmm, actually I thought to place the spinlock local because of having >> the problem just with the interrupt and not with mutex. >> >> But if global wouldn't it then better to make an array[MAX_DEV] for the >> lock with initialisation in _init or _start? > > Global means *one* spin-irq-lock for the indirect register access of all > devices. That might be the most efficient solution but we are sure that s/might/might not/ > it works, also with wired i82527 hardware, which seem to exist. That's > also what the related lincan and ocan drivers used. Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists