linux-kernel - Re: [PATCH 012/196] nozomi 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: <200801251344.30427.fseidel@suse.de>
Date:	Fri, 25 Jan 2008 13:44:29 +0100
From:	Frank Seidel <fseidel@...e.de>
To:	Jan Engelhardt <jengelh@...putergmbh.de>
Cc:	Greg Kroah-Hartman <gregkh@...e.de>, linux-kernel@...r.kernel.org,
	Jiri Slaby <jirislaby@...il.com>,
	Stefan Richter <stefanr@...6.in-berlin.de>
Subject: Re: [PATCH 012/196] nozomi driver

Hi,

thanks for your feedback.

On Freitag 25 Januar 2008 09:31:25, you (Jan Engelhardt) wrote:
> On Jan 24 2008 23:09, Greg Kroah-Hartman wrote:
> >+/*
> >+ * nozomi.c  -- HSDPA driver Broadband Wireless Data Card - Globe Trotter
> >+ *
> >+ * Written by: Ulf Jakobsson,
> >+ *             Jan �erfeldt,
>                      ^
> Neither in ISO-8859-1 nor UTF-8 this position contains something meaningful.
> Mind to update to UTF-8?

fixed

> >+/*
> >+ * CHANGELOG
> 
> Changelogs go into git, not files, at least that is what was mentioned
> time and again.

i removed them

> >+#ifdef __BIG_ENDIAN
> >+/* Big endian */
> >+
> >+struct toggles {
> >+	unsigned enabled:5;	/*
> >+				 * Toggle fields are valid if enabled is 0,
> >+				 * else A-channels must always be used.
> >+				 */
> >+	unsigned diag_dl:1;
> >+	unsigned mdm_dl:1;
> >+	unsigned mdm_ul:1;
> >+} __attribute__ ((packed));
> 
> Probably just me, unsigned int is a good bit more explicit.
> The driver may also use a combined struct with
> BIG_ENDIAN_BITFIELD/LITTLE_ENDIAN_BIGFIELD (see e.g. linux/ip.h).

Here i agree with Stefan, especially as this also would only
make a difference for one (struct config_table) of those four structs
and there it would vastly decrease readability.

> >+/*    Global variables */
> >+static struct pci_device_id nozomi_pci_tbl[] = {
> >+	{PCI_DEVICE(VENDOR1, DEVICE1)},
> >+	{},
> >+};
> 
> The macros are only used once, could just as well substitute them
> by their real values.

Yes, but on the other hand now all those driver specific numbers
are grouped together in the beginning of nozomi.c and it also
could happen there once will appear other cards or models of it
that this nozomi driver could handle.

> >+/*
> >+ * Handle donlink data, ports that are handled are modem and diagnostics
>             ^
> downlink

corrected

> >+static DEVICE_ATTR(card_type, 0444, card_type_show, NULL);
> 
> While 0444 is probably never going to change its meaning, S_IRUGO comes 
> into mind.

changed it to S_IRUGO

> >+/* just to discard single character writes */
> >+static void ntty_put_char(struct tty_struct *tty, unsigned char c)
> >+{
> >+	/* FIXME !!! */
> >+	DBG2("PUT CHAR Function: %c", c);
> >+}

Its just a misleading comment, but i changed it.

Besides that i also removed an error and a warning the current
checkpatch.pl now showed up ("trailing statments should be on next
line" and a "inline preferred over __inline-__").

Thanks,
Frank
--
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/