From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiner Kallweit Subject: Re: Issue with commit fea23fb591cc "net: phy: convert read-modify-write to phy_modify()" Date: Thu, 4 Jan 2018 20:16:44 +0100 Message-ID: References: <8cf4bdaa-bd05-1962-f1d0-6cb84db31588@googlemail.com> <20180104114439.GL28752@n2100.armlinux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Andrew Lunn , "David S. Miller" , "netdev@vger.kernel.org" To: Russell King - ARM Linux Return-path: Received: from mail-wm0-f66.google.com ([74.125.82.66]:45960 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752845AbeADTQ4 (ORCPT ); Thu, 4 Jan 2018 14:16:56 -0500 Received: by mail-wm0-f66.google.com with SMTP id 9so5314355wme.4 for ; Thu, 04 Jan 2018 11:16:55 -0800 (PST) In-Reply-To: <20180104114439.GL28752@n2100.armlinux.org.uk> Sender: netdev-owner@vger.kernel.org List-ID: Am 04.01.2018 um 12:44 schrieb Russell King - ARM Linux: > On Thu, Jan 04, 2018 at 08:00:53AM +0100, Heiner Kallweit wrote: >> Parameter mask of phy_modify() holds the bits to be cleared. >> In the mentioned commit parameter mask seems to be inverted in >> few cases, what IMO is wrong (see example). > > I'd be grateful if you could list those that you think are wrong please. > For function __phy_modify documentation and implementation conflict. Documentation states "(value & mask) | set" whilst implementation is "(value & ~mask) | set". Based on the subsequent patches I assume that your intention is what is documented. Personally I find "ret & ~mask" more intuitive (see also set_mask_bits in include/linux/bitops.h) but this may be a question of personal taste. In kernel code both flavors are used. + * Unlocked helper function which allows a PHY register to be modified as + * new register value = (old register value & mask) | set + */ +int __phy_modify(struct phy_device *phydev, u32 regnum, u16 mask, u16 set) +{ + int ret, res; + + ret = __phy_read(phydev, regnum); + if (ret >= 0) { + res = __phy_write(phydev, regnum, (ret & ~mask) | set); + if (res < 0) + ret = res; + } + + return ret; +} Could you please advise whether documentation or implementation reflect your intention? Then I'll check again which changes I'd consider to be wrong. Regards, Heiner >> Maybe I miss something, could you please check? > > It's entirely possible that some are wrong - the patch started out as > having the mask argument inverted, but during its evolution, that was > corrected, and I thought all places had been updated - maybe they were > initially wrong. > > I did go through the patch several times before sending it to try to > ensure that it was correct, but must have overlooked some, because the > one you quote is one I definitely looked at several times. It's highly > likely that if I have another look through the patch, I still won't > spot those that you've found. > >> And somehow related: >> When adding such helpers, wouldn't it make sense to add >> helpers for setting / clearing bits too? Something like: >> phy_set_bits(phydev, reg, val) -> phy_modify(phydev, reg, 0, val) > > Maybe, but lets try and solve the problems with the existing patch > first. > > Thanks for reporting this, and sorry for the hassle. >