linux-kernel - Re: [PATCH] mcs7830: Fix link state detection

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]

Date:	Thu, 11 Oct 2012 20:37:04 +0200
From:	Andreas Mohr <andi@...as.de>
To:	Ondrej Zary <linux@...nbow-software.org>
Cc:	Andreas Mohr <andi@...as.de>, Greg KH <gregkh@...uxfoundation.org>,
	davem@...emloft.net, Michael Leun <lkml20120218@...ton.leun.net>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mcs7830: Fix link state detection

Hi,

On Thu, Oct 11, 2012 at 12:33:28PM +0200, Ondrej Zary wrote:
>  	u8 *buf = urb->transfer_buffer;
>  	bool link;
> +	struct mcs7830_data *data = mcs7830_get_data(dev);
>  
>  	if (urb->actual_length < 16)
>  		return;

Alternatively could do *data = NULL; and then actually assign after the conditional.
But since the conditional most likely is coldpath
I think your chosen implementation is best.

>  
>  	link = !(buf[1] & 0x20);
>  	if (netif_carrier_ok(dev->net) != link) {

I usually like to introduce helper bools to clearly express the intention
behind things (source code should be readable like a book, yet it all
too often is everything but...).
I.e.
bool link_state_change_detected = (netif_carrier_ok......);
if (bool)

Might be an idea here, too.

The logic behind your counter implementation seems solid to me
(short explanatory comment "track link state several times, to guard against
transient erroneous link state of (some versions of?) this chip"
might have been useful though).

Thank you very much for your patch!

Andreas Mohr
--
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/