linux-kernel - Re: HSI: hsi: Introducing HSI framework

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-next>] [day] [month] [year] [list]

Date:	Mon, 2 Apr 2012 16:41:34 -0700
From:	Greg KH <greg@...ah.com>
To:	Carlos Chinea <carlos.chinea@...ia.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: HSI: hsi: Introducing HSI framework

On Mon, Apr 02, 2012 at 04:56:07PM +0000, Linux Kernel Mailing List wrote:
> +static void hsi_controller_release(struct device *dev __maybe_unused)
> +{
> +}
> +
> +static void hsi_port_release(struct device *dev __maybe_unused)
> +{
> +}

As per the documentation in the kernel tree, I get to mock you
mercilessly for doing something as foolish as this.  You are not smarter
than the kernel and don't think that you got rid of the kernel warning
properly by doing this.  Do you think that I wrote that code for no good
reason?  The kernel was being nice and telling you what you did wrong,
don't try to fake it out, it's smarter than you are here.

I also get to tell you that you need to fix this up right now, BEFORE
3.4 comes out.

And no, just because you created static struct devices, this is not ok,
DO NOT create static struct devices, that's foolish, and even worse, not
something you should be doing in the first place, create a real
structure, and put a struct device within it please.

And people wonder why kernel maintainers are grumpy.

> +/**
> + * struct hsi_client - HSI client attached to an HSI port
> + * @device: Driver model representation of the device
> + * @tx_cfg: HSI TX configuration
> + * @rx_cfg: HSI RX configuration
> + * @hsi_start_rx: Called after incoming wake line goes high
> + * @hsi_stop_rx: Called after incoming wake line goes low
> + */
> +struct hsi_client {
> +	struct device		device;
> +	struct hsi_config	tx_cfg;
> +	struct hsi_config	rx_cfg;
> +	void			(*hsi_start_rx)(struct hsi_client *cl);
> +	void			(*hsi_stop_rx)(struct hsi_client *cl);
> +	/* private: */
> +	unsigned int		pclaimed:1;
> +	struct list_head	link;
> +};

Why do you need another list_head in here?  What's wrong with using the
device iteration functions that are written for you to use?  Don't put
the structure on yet-another-list, use what is given to you please, as
odds are, you will traverse this list incorrectly (trust me...)

ugh,

greg k-h
--
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/