Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

[Andrew Lunn <andrew@lunn.ch>]

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