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: <20101224091428.GA375@e-circ.dyndns.org> Date: Fri, 24 Dec 2010 10:14:31 +0100 From: Kurt Van Dijck <kurt.van.dijck@....be> To: Marc Kleine-Budde <mkl@...gutronix.de> Cc: netdev@...r.kernel.org, socketcan-core@...ts.berlios.de Subject: Re: [PATCH net-next-2.6 1/2] can: add driver for Softing card Marc, A lot of your remarks do make sense, without further comment. Some however, I'm not completely sure ... On Thu, Dec 23, 2010 at 03:25:07PM +0100, Marc Kleine-Budde wrote: > > > > obj-y += usb/ > > +obj-y += softing/ > > I think it will (at least marginally) speed up the Kernel build process > only to dive into the softing subdir if Softing is enabled in Kconfig. Due to the independant driver design, I should (CONFIG_CAN_SOFTING || CONFIG_CAN_SOFTINGCS) > > > + ktime_t ts_ref; > > + ktime_t ts_overflow; /* timestamp overflow value, in ktime */ > > + > > + struct { > > + /* indication of firmware status */ > > + int up; > > + /* protection of the 'up' variable */ > > + struct mutex lock; > > + } fw; > > what about using an atomic_t for the firmware status? for 'up', yes, but the lock stays. It protects the startup/shutdown sequence too, ie. only 1 process enters the shutdown sequence. > > > +/* SOFTING DPRAM mappings */ > > +struct softing_rx { > > + u8 fifo[16][32]; > > + u8 dummy1; > > Just curious, why did they put a padding byte here, that makes the rest > unaligned? I did not design the DPRAM layout. It's just the way it is ... I did prefer to use structs in virtual memory, and this is the consequence. > > > + u32 time; > > + u32 time_wrap; > > + u8 wr_start; > > + u8 wr_end; > > + u8 dummy10; > > + u16 dummy12; > > + u16 dummy12x; > > + u16 dummy13; > > + u16 reset_rcv_fifo; > > + u8 dummy14; > > + u8 reset_xmt_fifo; > > + u8 read_fifo_levels; > > + u16 rcv_fifo_level; > > + u16 xmt_fifo_level; > > +} __attribute__((packed)); > > Can you renumber the dummy variables (there are some "x" in there), or > does it correspond to some datasheet? no, there's no datasheet. I started from code released by Softing. > > > + > > diff --git a/drivers/net/can/softing/softing_fw.c b/drivers/net/can/softing/softing_fw.c > > + [...] > > +int softing_fct_cmd(struct softing *card, int cmd, int vector, const char *msg) > > +{ > > + int ret; > > + unsigned long stamp; > > + if (vector == RES_OK) > > + vector = RES_NONE; > > + card->dpram.fct->param[0] = cmd; > > param[] is an array of s16 and cmd is an int. Is this a problem? Is it usefull to define the function with s16 arguments then? > > hmmm..all stuff behind dpram is __iomem, isn't it? I think it should > only be accessed with via the ioread/iowrite operators. Please check I did an ioremap_nocache. Since it is unaligned, ioread/iowrite would render a lot of statements. > your code with sparse (compile with "make C=2"). (?) > > > + } > > + if ((jiffies - stamp) >= 1 * HZ) > > That's not good. I don't remember the name, but there are some > functions/defines to do this kind of things properly. I'll do a search > > > + break; > > + if (in_interrupt()) > > + /* go as fast as possible */ > > In the worst case this means you lock up the system for one second. Does > the card issue an interrupt if it's finished? Another option is to write > a threaded interrupt handler. Yep, threaded interrupt handler is something to look at ... later. > > > > +{ > > + int ret; > > + unsigned long stamp; > > + card->dpram.receipt[0] = RES_NONE; > > + card->dpram.command[0] = command; > > + /* be sure to flush this to the card */ > > + wmb(); > > + stamp = jiffies; > > + /*wait for card */ > > + do { > > + ret = card->dpram.receipt[0]; > > + /* don't have any cached variables */ > > + rmb(); > > + if (ret == RES_OK) > > + return 0; > > + if ((jiffies - stamp) >= (3 * HZ)) > > + break; > > + schedule(); > > same applies here, too. Although this command seems not to be called > from interrupt context, what about using a msleep() instead of a schedule? Not calling schedule was really annoying. > Thanks for your review, Kurt -- 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