linux-kernel - Re: [spi-devel-general] [PATCH RESEND] max3100 driver

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: <200809242156.29505.david-b@pacbell.net>
Date:	Wed, 24 Sep 2008 21:56:29 -0700
From:	David Brownell <david-b@...bell.net>
To:	Christian Pellegrin <chripell@...il.com>
Cc:	spi-devel-general@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	Christian Pellegrin <chripell@...e.org>,
	alan@...rguk.ukuu.org.uk, linux-serial@...r.kernel.org
Subject: Re: [spi-devel-general] [PATCH RESEND] max3100 driver

You got this working ... congrats!  :)

Small suggestion:  next time you resend this, make sure
that $SUBJECT mentions it's a UART driver.  Maybe even that
it's a SPI UART driver.  That should help get more comments.

On Saturday 20 September 2008, Christian Pellegrin wrote:
> +struct plat_max3100 {
> +/* force MAX3100 in loopback */
> +       int loopback;
> +/* 0 for 3.6864 Mhz, 1 for 1.8432  */
> +       int crystal;
> +/* for archs like PXA with only edge irqs */
> +       int only_edge_irq;
> +/* MAX3100 has a shutdown pin. This is a hook
> +   called on suspend and resume to activate it.*/
> +       void (*max3100_hw_suspend) (int suspend);
> +/* poll time for ctr signals in ms, 0 disables (so no hw flow
> + * ctrl is possible)  */
> +       int poll_time;
> +};

This is a bit picky, but it's the first thing I noticed when
scanning the patch ... wierd comment layout!  Either indent
those all, or (better) convert to kerneldoc style.

Potentially less picky:  probe() doesn't lock max3100s[],
neither does remove(), and in fact there seems to be no
lock for that table.  Which suggests trouble in cases like
concurrent I/O (including open) and driver remove().  You
should probably just allocate a mutex to help control that
table, like most other drivers.

And is that workqueue single threaded?

I just skimmed the rest of the driver.  Seems to be
fairly straightforward -- at least the non-TTY bits.
--
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/