From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Lunn <andrew@lunn.ch>
Subject: Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver
Date: Sun, 23 Dec 2018 10:59:40 +0100
Message-ID: <20181223095940.GA31681@lunn.ch>
References: <20181221233552.3741-1-marex@denx.de>
 <836d9cec-0175-3cd1-b59b-2021195b8461@gmail.com>
 <9ae94569-2bef-acea-6b48-18d679060901@denx.de>
 <6f19cf67-aef8-9b9c-1993-bf986bb8d41f@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Marek Vasut <marex@denx.de>, netdev@vger.kernel.org,
        Florian Fainelli <f.fainelli@gmail.com>
To: Heiner Kallweit <hkallweit1@gmail.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from vps0.lunn.ch ([185.16.172.187]:56085 "EHLO vps0.lunn.ch"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1725790AbeLWJ7n (ORCPT <rfc822;netdev@vger.kernel.org>);
        Sun, 23 Dec 2018 04:59:43 -0500
Content-Disposition: inline
In-Reply-To: <6f19cf67-aef8-9b9c-1993-bf986bb8d41f@gmail.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

> >>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >>> +	if (!priv)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
> >>> +	if (!priv->hwmon_name)
> >>> +		return -ENODEV;
> >>
> >> Do you really need to make a copy of the device name?
> >> Why not simply priv->hwmon_name = dev_name(dev) ?
> > 
> > Fine by me, but then maybe I don't quite understand why the other
> > drivers duplicate the name, eg. the sfp.c one.
> > 
> It's a question of object lifetime. If the original object can go away
> before your object, then you need to make a copy of the name.
> However in our case I don't think priv can live longer than dev.
> 
> >> And if devm_kstrdup fails, then most likely you have an out-of-memory
> >> error, so why not return -ENOMEM as usual?
> > 
> > Fixed
> > 
> >>> +
> >>> +	for (i = 0; priv->hwmon_name[i]; i++)
> >>> +		if (hwmon_is_bad_char(priv->hwmon_name[i]))
> >>> +			priv->hwmon_name[i] = '_';

This is one reason to make a copy. You don't want to apply that to
main name of the device.

> >>> +
> >>> +	priv->hwmon_dev =
> >>> +		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
> >>> +						     phydev,
> >>> +						     &tja11xx_hwmon_chip_info,
> >>> +						     NULL);
> >>> +

The second reason is priv is released before dev, but what about
hwmon, especially if somebody has one of the files open? Is the
unregister synchronous?

	 Andrew