netdev - Re: [PATCH] phylib: Add support for the LXT973 phy.

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:	Wed, 2 Jun 2010 14:32:11 -0500
From:	Andy Fleming <afleming@...il.com>
To:	David Miller <davem@...emloft.net>
Cc:	richardcochran@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH] phylib: Add support for the LXT973 phy.

On Wed, Jun 2, 2010 at 8:50 AM, David Miller <davem@...emloft.net> wrote:
> From: Richard Cochran <richardcochran@...il.com>
> Date: Wed, 2 Jun 2010 14:55:27 +0200
>
>> On Tue, Jun 01, 2010 at 05:39:22PM -0500, Andy Fleming wrote:
>>> That's a bit hacky.  There is a dev_flags field, which could be used
>>> for this.  Probably, we should add a more general way of saying what
>>> sort of port this is.  But don't use the presence and absence of
>>> "priv", as it could one day get used for a different purpose, and this
>>> seems like it would leave us open to strange bugs.
>>
>> Okay, I changed it.
>>
>> At first, I was worried about using 'dev_flags' because I couldn't
>> tell exactly who may write to this field. Looking at tg.c and
>> broadcom.c, it appears that the MAC drivers may also write this
>> field. In contrast, the 'priv' field is surely private.
>
> No, I think using dev_flags is absolutely the wrong way to do about
> this.

Yeah, I was clearly not thinking clearly.  dev_flags will be
overwritten, and is not meant for this.  I believe, what we should do
is add a "port" field to the PHY device, and if PCR_FIBER_SELECT is
set, then set the port field to PORT_FIBRE.  I'm not entirely clear on
the semantics of that field in the ethtool cmd, but it seems like the
right idea.

>
> phy_device->priv "could one day get used for a different purpose"?
> What in the world are you smoking Andy?
>
> It's clearly a private state pointer for the PHY driver to use,
> full stop.  There is absolutely no ambiguity of what this value
> is and what it is used for and who owns it.  The comments in the
> layout of struct phy_device state this clearly as well.

Yes, it's private, and I would be fine with using priv, I just didn't
think that it was correct to assign priv to point at the probe
function as a mechanism for indicating that fiber is being used.  I
believe that code should be more explicit than that.  priv is meant to
point at private data, not to indicate an arbitrary attribute of the
link by virtue of being non-null.

Andy
--
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