Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1517185 > unrolled thread

Re: [alsa-devel] [PATCH 2/9] ALSA: ac97: add an ac97 bus

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [alsa-devel] [PATCH 2/9] ALSA: ac97: add an ac97 bus Lars-Peter Clausen <lars@metafoo.de> - 2016-11-08 14:40 +0100
    Re: [alsa-devel] [PATCH 2/9] ALSA: ac97: add an ac97 bus Robert Jarzmik <robert.jarzmik@free.fr> - 2016-11-08 22:30 +0100
      Re: [alsa-devel] [PATCH 2/9] ALSA: ac97: add an ac97 bus Lars-Peter Clausen <lars@metafoo.de> - 2016-11-09 14:20 +0100
        Re: [alsa-devel] [PATCH 2/9] ALSA: ac97: add an ac97 bus Robert Jarzmik <robert.jarzmik@free.fr> - 2016-11-09 22:30 +0100
          Re: [alsa-devel] [PATCH 2/9] ALSA: ac97: add an ac97 bus Lars-Peter Clausen <lars@metafoo.de> - 2016-11-10 12:40 +0100

#1517185 — Re: [alsa-devel] [PATCH 2/9] ALSA: ac97: add an ac97 bus

On 10/26/2016 09:41 PM, Robert Jarzmik wrote:
> AC97 is a bus for sound usage. It enables for a AC97 AC-Link to link one
> controller to 0 to 4 AC97 codecs.
> 
> The goal of this new implementation is to implement a device/driver
> model for AC97, with an automatic scan of the bus and automatic
> discovery of AC97 codec devices.
> 

Good work, a couple of comments inline.

[...]
> diff --git a/include/sound/ac97/codec.h b/include/sound/ac97/codec.h
> new file mode 100644
> index 000000000000..8901c1200522
> --- /dev/null
> +++ b/include/sound/ac97/codec.h
> @@ -0,0 +1,115 @@
> +/*
> + *  Copyright (C) 2016 Robert Jarzmik <robert.jarzmik@free.fr>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef __SOUND_AC97_CODEC2_H
> +#define __SOUND_AC97_CODEC2_H
> +
> +#include <linux/device.h>
> +
> +#define AC97_ID(vendor_id1, vendor_id2) \
> +	((((vendor_id1) & 0xffff) << 16) | ((vendor_id2) & 0xffff))
> +#define AC97_DRIVER_ID(vendor_id1, vendor_id2, mask_id1, mask_id2, _data) \
> +	{ .id = (((vendor_id1) & 0xffff) << 16) | ((vendor_id2) & 0xffff), \
> +	  .mask = (((mask_id1) & 0xffff) << 16) | ((mask_id2) & 0xffff), \
> +	  .data = (_data) }
> +
> +#define to_ac97_device(d) container_of(d, struct ac97_codec_device, dev)
> +#define to_ac97_driver(d) container_of(d, struct ac97_codec_driver, driver)

In my opinion these should be inline functions rather than macros as that
generates much more legible compiler errors e.g. in case there is a type
mismatch.

[...]
> +struct ac97_codec_driver {
> +	struct device_driver	driver;
> +	int			(*probe)(struct ac97_codec_device *);
> +	int			(*remove)(struct ac97_codec_device *);
> +	int			(*suspend)(struct ac97_codec_device *);
> +	int			(*resume)(struct ac97_codec_device *);
> +	void			(*shutdown)(struct ac97_codec_device *);

The suspend, resume and shutdown callbacks are never used. Which is good,
since all new frameworks should use dev_pm_ops. Just drop the from the struct.

> +	const struct ac97_id	*id_table;
> +};
[...]
> diff --git a/include/sound/ac97/controller.h b/include/sound/ac97/controller.h
> new file mode 100644
> index 000000000000..5ff59bd7e324
> --- /dev/null
> +++ b/include/sound/ac97/controller.h
> @@ -0,0 +1,85 @@
> +/*
> + *  Copyright (C) 2016 Robert Jarzmik <robert.jarzmik@free.fr>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef AC97_CONTROLLER_H
> +#define AC97_CONTROLLER_H
> +
> +#include <linux/list.h>
> +
> +#define AC97_BUS_MAX_CODECS 4
> +#define AC97_SLOTS_AVAILABLE_ALL 0xf
> +
> +struct device;
> +
> +/**
> + * struct ac97_controller - The AC97 controller of the AC-Link
> + * @ops:		the AC97 operations.
> + * @controllers:	linked list of all existing controllers.
> + * @dev:		the device providing the AC97 controller.
> + * @slots_available:	the mask of accessible/scanable codecs.
> + * @codecs:		the 4 possible AC97 codecs (NULL if none found).
> + * @codecs_pdata:	platform_data for each codec (NULL if no pdata).
> + *
> + * This structure is internal to AC97 bus, and should not be used by the
> + * controllers themselves, excepting for using @dev.
> + */
> +struct ac97_controller {
> +	const struct ac97_controller_ops *ops;
> +	struct list_head controllers;
> +	struct device *dev;

I'd make the controller itself a struct dev, rather than just having the
pointer to the parent. This is more idiomatic and matches what other
subsystems do. It has several advantages, you get proper refcounting of your
controller structure, the controller gets its own sysfs directory where the
CODECs appear as children, you don't need the manual sysfs attribute
creation and removal in ac97_controler_{un,}register().

> +	unsigned short slots_available;
> +	struct ac97_codec_device *codecs[AC97_BUS_MAX_CODECS];

If you make the controller a struct dev you can also remove this, since the
device driver core tracks the children of a device.

> +	void *codecs_pdata[AC97_BUS_MAX_CODECS];
> +};
> +
> +/**
> + * struct ac97_controller_ops - The AC97 operations
> + * @reset:	Cold reset of the AC97 AC-Link.
> + * @warm_reset:	Warm reset of the AC97 AC-Link.
> + * @read:	Read of a single AC97 register.
> + *		Returns the register value or a negative error code.
> + * @write:	Write of a single AC97 register.
> + * @wait:	Wait for the current AC97 operation to finish (might be NULL).
> + * @init:	Initialization of the AC97 AC-Link (might be NULL).
> + *
> + * These are the basic operation an AC97 controller must provide for an AC97
> + * access functions. Amongst these, all but the last 2 are mandatory.
> + * The slot number is also known as the AC97 codec number, between 0 and 3.
> + */
> +struct ac97_controller_ops {
> +	void (*reset)(struct ac97_controller *adrv);
> +	void (*warm_reset)(struct ac97_controller *adrv);
> +	int (*write)(struct ac97_controller *adrv, int slot,
> +		     unsigned short reg, unsigned short val);
> +	int (*read)(struct ac97_controller *adrv, int slot, unsigned short reg);
> +	void (*wait)(struct ac97_controller *adrv, int slot);
> +	void (*init)(struct ac97_controller *adrv, int slot);

Neither wait nor init are ever used.

> +};
> +
[...]
> +/*
> + * Protects ac97_controllers and each ac97_controller structure.
> + */
> +static DEFINE_MUTEX(ac97_controllers_mutex);
> +static LIST_HEAD(ac97_controllers);
> +
> +static struct bus_type ac97_bus_type;
> +
> +static struct ac97_codec_device *
> +ac97_codec_find(struct ac97_controller *ac97_ctrl, int codec_num)

unsigned int codec_num

> +{
> +	if ((codec_num < 0) || (codec_num >= AC97_BUS_MAX_CODECS))
> +		return ERR_PTR(-ERANGE);

I'd make this EINVAL.

> +
> +	return ac97_ctrl->codecs[codec_num];
> +}
> +
> +static void ac97_codec_release(struct device *dev)
> +{
> +	struct ac97_codec_device *adev;
> +	struct ac97_controller *ac97_ctrl;
> +
> +	adev = container_of(dev, struct ac97_codec_device, dev);

to_ac97_device()

> +	ac97_ctrl = adev->ac97_ctrl;
> +	ac97_ctrl->codecs[adev->num] = NULL;
> +	sysfs_remove_link(&dev->kobj, "ac97_controller");
> +	kfree(adev);
> +}
> +
> +static int ac97_codec_add(struct ac97_controller *ac97_ctrl, int idx,
> +		   unsigned int vendor_id)
> +{
> +	struct ac97_codec_device *codec;
> +	char *codec_name;
> +	int ret;
> +
> +	codec = kzalloc(sizeof(*codec), GFP_KERNEL);
> +	if (!codec)
> +		return -ENOMEM;
> +	ac97_ctrl->codecs[idx] = codec;
> +	codec->vendor_id = vendor_id;
> +	codec->dev.release = ac97_codec_release;
> +	codec->dev.bus = &ac97_bus_type;
> +	codec->dev.parent = ac97_ctrl->dev;
> +	codec->num = idx;
> +	codec->ac97_ctrl = ac97_ctrl;
> +
> +	codec_name = kasprintf(GFP_KERNEL, "%s:%d", dev_name(ac97_ctrl->dev),
> +			       idx);
> +	codec->dev.init_name = codec_name;

init_name is only for statically allocated devices. Use dev_set_name(dev,
...). No need for kasprintf() either as dev_set_name() takes a format string.

For this you need to split device_register into device_initialize() and
device_add(). But usually that is what you want anyway.

> +
> +	ret = device_register(&codec->dev);
> +	kfree(codec_name);
> +	if (ret)
> +		goto err_free_codec;
> +
> +	ret = sysfs_create_link(&codec->dev.kobj, &ac97_ctrl->dev->kobj,
> +				"ac97_controller");

Since the CODEC is a child of the controller this should not be necessary as
this just points one directory up. It's like `ln -s .. parent`

> +	if (ret)
> +		goto err_unregister_device;
> +
> +	return 0;
> +err_unregister_device:
> +	put_device(&codec->dev);
> +err_free_codec:
> +	kfree(codec);

Since the struct is reference counted, the freeing is done in the release
callback and this leads to a double free.

> +	ac97_ctrl->codecs[idx] = NULL;
> +
> +	return ret;
> +}
[...]
> +/**
> + * snd_ac97_codec_driver_register - register an AC97 codec driver
> + * @dev: AC97 driver codec to register
> + *
> + * Register an AC97 codec driver to the ac97 bus driver, aka. the AC97 digital
> + * controller.
> + *
> + * Returns 0 on success or error code
> + */
> +int snd_ac97_codec_driver_register(struct ac97_codec_driver *drv)
> +{
> +	int ret;
> +
> +	drv->driver.bus = &ac97_bus_type;
> +
> +	ret = driver_register(&drv->driver);
> +	if (!ret)
> +		ac97_rescan_all_controllers();

Rescanning the bus when a new codec driver is registered should not be
neccessary. The bus is scanned once when the controller is registered, this
creates the device. The device driver core will take care of binding the
device to the driver, if the driver is registered after thed evice.

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(snd_ac97_codec_driver_register);
> +
[...]
> +static int ac97_ctrl_codecs_unregister(struct ac97_controller *ac97_ctrl)
> +{
> +	int i;
> +
> +	for (i = 0; i < AC97_BUS_MAX_CODECS; i++)
> +		if (ac97_ctrl->codecs[i])
> +			put_device(&ac97_ctrl->codecs[i]->dev);

This should be device_unregister() to match the device_register() in
ac97_codec_add().

> +
> +	return 0;
> +}
> +
> +static ssize_t cold_reset_store(struct device *dev,
> +				struct device_attribute *attr, const char *buf,
> +				size_t len)
> +{
> +	struct ac97_controller *ac97_ctrl = ac97_ctrl_find(dev);
> +
> +	if (!dev)
> +		return -ENODEV;

dev is never NULL here. And for the ac97_ctrl there is a race condition. It
could be unregistered and freed after ac97_ctrl_find() returned sucessfully,
but before ac97_ctrl->ops is used.

> +
> +	ac97_ctrl->ops->reset(ac97_ctrl);
> +	return len;
> +}
> +static DEVICE_ATTR_WO(cold_reset);
> +
> +static ssize_t warm_reset_store(struct device *dev,
> +				struct device_attribute *attr, const char *buf,
> +				size_t len)
> +{
> +	struct ac97_controller *ac97_ctrl = ac97_ctrl_find(dev);
> +
> +	if (!dev)
> +		return -ENODEV;

Same here.

> +
> +	ac97_ctrl->ops->warm_reset(ac97_ctrl);
> +	return len;
> +}
> +static DEVICE_ATTR_WO(warm_reset);
> +
> +static struct attribute *ac97_controller_device_attrs[] = {
> +	&dev_attr_cold_reset.attr,
> +	&dev_attr_warm_reset.attr,
> +	NULL
> +};

This adds new userspace ABI that is not documented at the moment.

> +
> +static const struct attribute_group ac97_controller_attr_group = {
> +	.name	= "ac97_operations",
> +	.attrs	= ac97_controller_device_attrs,
> +};
> +
> +/**
> + * snd_ac97_controller_register - register an ac97 controller
> + * @ops: the ac97 bus operations
> + * @dev: the device providing the ac97 DC function
> + * @slots_available: mask of the ac97 codecs that can be scanned and probed
> + *                   bit0 => codec 0, bit1 => codec 1 ... bit 3 => codec 3
> + *
> + * Register a digital controller which can control up to 4 ac97 codecs. This is
> + * the controller side of the AC97 AC-link, while the slave side are the codecs.
> + *
> + * Returns 0 upon success, negative value upon error
> + */
> +int snd_ac97_controller_register(const struct ac97_controller_ops *ops,
> +				 struct device *dev,
> +				 unsigned short slots_available,
> +				 void **codecs_pdata)

In my opinion this should return a handle to a ac97 controller which can
then be passed to snd_ac97_controller_unregister(). This is in my opinion
the better approach rather than looking up the controller by parent device.

> +{
> +	struct ac97_controller *ac97_ctrl;
> +	int ret, i;
> +
> +	ac97_ctrl = kzalloc(sizeof(*ac97_ctrl), GFP_KERNEL);
> +	if (!ac97_ctrl)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < AC97_BUS_MAX_CODECS && codecs_pdata; i++)
> +		ac97_ctrl->codecs_pdata[i] = codecs_pdata[i];
> +
> +	ret = sysfs_create_group(&dev->kobj, &ac97_controller_attr_group);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&ac97_controllers_mutex);
> +	ac97_ctrl->ops = ops;
> +	ac97_ctrl->slots_available = slots_available;
> +	ac97_ctrl->dev = dev;
> +	list_add(&ac97_ctrl->controllers, &ac97_controllers);

Stricly speeaking only the list_add needs to be protected.

> +	mutex_unlock(&ac97_controllers_mutex);
> +
> +	ac97_bus_reset(ac97_ctrl);
> +	ac97_bus_scan(ac97_ctrl);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(snd_ac97_controller_register);
> +
> +/**
> + * snd_ac97_controller_unregister - unregister an ac97 controller
> + * @dev: the device previously provided to ac97_controller_register()
> + *
> + * Returns 0 on success, negative upon error

Unregister must not be able to fail. Hotunplug is one of the core concepts
of the device driver model and there is really nothing that can be done to
prevent a device from disappearing, so there is no sensible way of handling
the error (and your pxa driver modifications simply ignore it as well).

This also means the framework needs to cope with the case where the
controller is removed and the CODEC devices are still present. All future
operations should return -ENODEV in that case.

> + */
> +int snd_ac97_controller_unregister(struct device *dev)
> +{
> +	struct ac97_controller *ac97_ctrl;
> +	int ret = -ENODEV, i;
> +
> +	mutex_lock(&ac97_controllers_mutex);
> +	ac97_ctrl = ac97_ctrl_find(dev);
> +	if (ac97_ctrl) {
> +		ret = 0;
> +		for (i = 0; i < AC97_BUS_MAX_CODECS; i++)
> +			if (ac97_ctrl->codecs[i] &&
> +			    device_is_registered(&ac97_ctrl->codecs[i]->dev))
> +				ret = -EBUSY;
> +		if (!ret)
> +			ret = ac97_ctrl_codecs_unregister(ac97_ctrl);
> +		if (!ret) {
> +			list_del(&ac97_ctrl->controllers);
> +			sysfs_remove_group(&dev->kobj,
> +					   &ac97_controller_attr_group);
> +		}
> +	}
> +	mutex_unlock(&ac97_controllers_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(snd_ac97_controller_unregister);
[...]
> +static struct bus_type ac97_bus_type = {
> +	.name		= "ac97",
> +	.dev_attrs	= ac97_dev_attrs,

dev_attrs is deprecated in favor of dev_groups (See commit 880ffb5c6).

> +	.match		= ac97_bus_match,
> +	.pm		= &ac97_pm,
> +	.probe		= ac97_bus_probe,
> +	.remove		= ac97_bus_remove,
> +};
> +
> +static int __init ac97_bus_init(void)
> +{
> +	return bus_register(&ac97_bus_type);
> +}
> +subsys_initcall(ac97_bus_init);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Robert Jarzmik <robert.jarzmik@free.fr>");
> diff --git a/sound/ac97/codec.c b/sound/ac97/codec.c
> new file mode 100644
> index 000000000000..a835f03744bf
> --- /dev/null
> +++ b/sound/ac97/codec.c
> @@ -0,0 +1,15 @@
> +/*
> + *  Copyright (C) 2016 Robert Jarzmik <robert.jarzmik@free.fr>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <sound/ac97_codec.h>
> +#include <sound/ac97/codec.h>
> +#include <sound/ac97/controller.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <sound/soc.h>	/* For compat_ac97_* */
> +

I'm not sure I understand what this file does.

[...]

[toc] | [next] | [standalone]

#1517596

Lars-Peter Clausen <lars@metafoo.de> writes:

> On 10/26/2016 09:41 PM, Robert Jarzmik wrote:
>> +#define to_ac97_device(d) container_of(d, struct ac97_codec_device, dev)
>> +#define to_ac97_driver(d) container_of(d, struct ac97_codec_driver, driver)
>
> In my opinion these should be inline functions rather than macros as that
> generates much more legible compiler errors e.g. in case there is a type
> mismatch.
Sure, why not.

>> +struct ac97_codec_driver {
>> +	struct device_driver	driver;
>> +	int			(*probe)(struct ac97_codec_device *);
>> +	int			(*remove)(struct ac97_codec_device *);
>> +	int			(*suspend)(struct ac97_codec_device *);
>> +	int			(*resume)(struct ac97_codec_device *);
>> +	void			(*shutdown)(struct ac97_codec_device *);
>
> The suspend, resume and shutdown callbacks are never used. Which is good,
> since all new frameworks should use dev_pm_ops. Just drop the from the struct.
Ok.

>> +/**
>> + * struct ac97_controller - The AC97 controller of the AC-Link
>> + * @ops:		the AC97 operations.
>> + * @controllers:	linked list of all existing controllers.
>> + * @dev:		the device providing the AC97 controller.
>> + * @slots_available:	the mask of accessible/scanable codecs.
>> + * @codecs:		the 4 possible AC97 codecs (NULL if none found).
>> + * @codecs_pdata:	platform_data for each codec (NULL if no pdata).
>> + *
>> + * This structure is internal to AC97 bus, and should not be used by the
>> + * controllers themselves, excepting for using @dev.
>> + */
>> +struct ac97_controller {
>> +	const struct ac97_controller_ops *ops;
>> +	struct list_head controllers;
>> +	struct device *dev;
>
> I'd make the controller itself a struct dev, rather than just having the
> pointer to the parent. This is more idiomatic and matches what other
> subsystems do. It has several advantages, you get proper refcounting of your
> controller structure, the controller gets its own sysfs directory where the
> CODECs appear as children, you don't need the manual sysfs attribute
> creation and removal in ac97_controler_{un,}register().

If you mean having "struct device dev" instead of "struct device *dev", it has
also a drawback : all the legacy platforms have already a probing method, be
that platform data, device-tree or something else.

I'm a bit converned about the conversion toll. Maybe I've not understood your
point fully, so please feel free to explain, with an actual example even better.

>> +	void (*wait)(struct ac97_controller *adrv, int slot);
>> +	void (*init)(struct ac97_controller *adrv, int slot);
>
> Neither wait nor init are ever used.
This is because I've not begun to porting sound/pci/ac97_codec.c into
sound/ac97.

>> +	if ((codec_num < 0) || (codec_num >= AC97_BUS_MAX_CODECS))
>> +		return ERR_PTR(-ERANGE);
>
> I'd make this EINVAL.
Ok.

>> +	adev = container_of(dev, struct ac97_codec_device, dev);
>
> to_ac97_device()
Sure.

>> +	codec_name = kasprintf(GFP_KERNEL, "%s:%d", dev_name(ac97_ctrl->dev),
>> +			       idx);
>> +	codec->dev.init_name = codec_name;
>
> init_name is only for statically allocated devices. Use dev_set_name(dev,
> ...). No need for kasprintf() either as dev_set_name() takes a format string.
I'll try again, I seem to remember having tried that and it failed, but I can't
remember why.

> For this you need to split device_register into device_initialize() and
> device_add(). But usually that is what you want anyway.
Let me try again.

>> +	ret = sysfs_create_link(&codec->dev.kobj, &ac97_ctrl->dev->kobj,
>> +				"ac97_controller");
>
> Since the CODEC is a child of the controller this should not be necessary as
> this just points one directory up. It's like `ln -s .. parent`
This creates :
/sys/bus/ac97/devices/pxa2xx-ac97\:0/ac97_controller

Of course as you pointed out, it's a 'ln -s .. parent' like link, but it seems
to me very unfriendly to have :
 - /sys/bus/ac97/devices/pxa2xx-ac97\:0/../ac97_operations
 - while /sys/bus/ac97/devices/ac97_operations doesn't exist (obviously)

Mmm, I don't know for this one, my mind is not set, it's a bit hard to tell if
it's only an unecessary link bringing "comfort", or something usefull.

>
>> +	if (ret)
>> +		goto err_unregister_device;
>> +
>> +	return 0;
>> +err_unregister_device:
>> +	put_device(&codec->dev);
>> +err_free_codec:
>> +	kfree(codec);
>
> Since the struct is reference counted, the freeing is done in the release
> callback and this leads to a double free.
A yes in the "goto err_unregister_device" case right, while it's necessary in
the "goto err_free_codec" case ... I need to rework that a bit.

>> +int snd_ac97_codec_driver_register(struct ac97_codec_driver *drv)
>> +{
>> +	int ret;
>> +
>> +	drv->driver.bus = &ac97_bus_type;
>> +
>> +	ret = driver_register(&drv->driver);
>> +	if (!ret)
>> +		ac97_rescan_all_controllers();
>
> Rescanning the bus when a new codec driver is registered should not be
> neccessary. The bus is scanned once when the controller is registered, this
> creates the device. The device driver core will take care of binding the
> device to the driver, if the driver is registered after thed evice.
That's because you suppose the initial scanning found all the ac97 codecs.
But that's an incomplete vision as a codec might be powered off at that time and
not seen by the first scanning, while the new scanning will discover it.

>> +static int ac97_ctrl_codecs_unregister(struct ac97_controller *ac97_ctrl)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < AC97_BUS_MAX_CODECS; i++)
>> +		if (ac97_ctrl->codecs[i])
>> +			put_device(&ac97_ctrl->codecs[i]->dev);
>
> This should be device_unregister() to match the device_register() in
> ac97_codec_add().
Right.

>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t cold_reset_store(struct device *dev,
>> +				struct device_attribute *attr, const char *buf,
>> +				size_t len)
>> +{
>> +	struct ac97_controller *ac97_ctrl = ac97_ctrl_find(dev);
>> +
>> +	if (!dev)
>> +		return -ENODEV;
>
> dev is never NULL here.
Ok.

> And for the ac97_ctrl there is a race condition. It could be unregistered and
> freed after ac97_ctrl_find() returned sucessfully, but before ac97_ctrl->ops
> is used.
A good catch, the ac97_controllers_mutex is missing.

> Same here.
Indeed.

>> +
>> +	ac97_ctrl->ops->warm_reset(ac97_ctrl);
>> +	return len;
>> +}
>> +static DEVICE_ATTR_WO(warm_reset);
>> +
>> +static struct attribute *ac97_controller_device_attrs[] = {
>> +	&dev_attr_cold_reset.attr,
>> +	&dev_attr_warm_reset.attr,
>> +	NULL
>> +};
>
> This adds new userspace ABI that is not documented at the moment.
Very true. And as all userspace interface, it needs to be discussed. If nobody
complains, I'll add the documentation for next patch round.
>
>> +int snd_ac97_controller_register(const struct ac97_controller_ops *ops,
>> +				 struct device *dev,
>> +				 unsigned short slots_available,
>> +				 void **codecs_pdata)
>
> In my opinion this should return a handle to a ac97 controller which can
> then be passed to snd_ac97_controller_unregister(). This is in my opinion
> the better approach rather than looking up the controller by parent device.
This is another "legacy drivers" issue.

The legacy driver (the one probed by platform_data or devicetree) doesn't
necessarily have a private structure to store this ac97_controller pointer. This
enables an "easier" porting of existing drivers.

>> +/**
>> + * snd_ac97_controller_unregister - unregister an ac97 controller
>> + * @dev: the device previously provided to ac97_controller_register()
>> + *
>> + * Returns 0 on success, negative upon error
>
> Unregister must not be able to fail. Hotunplug is one of the core concepts
> of the device driver model and there is really nothing that can be done to
> prevent a device from disappearing, so there is no sensible way of handling
> the error (and your pxa driver modifications simply ignore it as well).
>
> This also means the framework needs to cope with the case where the
> controller is removed and the CODEC devices are still present. All future
> operations should return -ENODEV in that case.
Ah ... that will require a serious modification, and I see your point, so I'll
prepare this for next patchset.
>> +static struct bus_type ac97_bus_type = {
>> +	.name		= "ac97",
>> +	.dev_attrs	= ac97_dev_attrs,
>
> dev_attrs is deprecated in favor of dev_groups (See commit 880ffb5c6).
Ok.

>> index 000000000000..a835f03744bf
>> --- /dev/null
>> +++ b/sound/ac97/codec.c
>> @@ -0,0 +1,15 @@
>> +/*
>> + *  Copyright (C) 2016 Robert Jarzmik <robert.jarzmik@free.fr>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <sound/ac97_codec.h>
>> +#include <sound/ac97/codec.h>
>> +#include <sound/ac97/controller.h>
>> +#include <linux/device.h>
>> +#include <linux/slab.h>
>> +#include <sound/soc.h>	/* For compat_ac97_* */
>> +
>
> I'm not sure I understand what this file does.
Ah yes, I'll remove it.
It's the future conversion of sound/pci/ac97_codec.c, but it's ... empty so far.

Thanks very much for the very detailed review.

-- 
Robert

[toc] | [prev] | [next] | [standalone]

#1518136

On 11/08/2016 10:18 PM, Robert Jarzmik wrote:
[...]
>>> +/**
>>> + * struct ac97_controller - The AC97 controller of the AC-Link
>>> + * @ops:		the AC97 operations.
>>> + * @controllers:	linked list of all existing controllers.
>>> + * @dev:		the device providing the AC97 controller.
>>> + * @slots_available:	the mask of accessible/scanable codecs.
>>> + * @codecs:		the 4 possible AC97 codecs (NULL if none found).
>>> + * @codecs_pdata:	platform_data for each codec (NULL if no pdata).
>>> + *
>>> + * This structure is internal to AC97 bus, and should not be used by the
>>> + * controllers themselves, excepting for using @dev.
>>> + */
>>> +struct ac97_controller {
>>> +	const struct ac97_controller_ops *ops;
>>> +	struct list_head controllers;
>>> +	struct device *dev;
>>
>> I'd make the controller itself a struct dev, rather than just having the
>> pointer to the parent. This is more idiomatic and matches what other
>> subsystems do. It has several advantages, you get proper refcounting of your
>> controller structure, the controller gets its own sysfs directory where the
>> CODECs appear as children, you don't need the manual sysfs attribute
>> creation and removal in ac97_controler_{un,}register().
> 
> If you mean having "struct device dev" instead of "struct device *dev", it has
> also a drawback : all the legacy platforms have already a probing method, be
> that platform data, device-tree or something else.
> 
> I'm a bit converned about the conversion toll. Maybe I've not understood your
> point fully, so please feel free to explain, with an actual example even better.

This would be a struct device that is not bound to a driver, but just acts
as a shell for the controller and places it inside the device hierarchy. You
get reference counting and other management functions as well as a
consistent naming scheme. E.g. you can call the devices ac97c%d (or
something similar) and then call the CODEC ac97c%d.%d.

This is how most frameworks implementing some kind of control bus are
structured in the Linux kernel. E.g. take a look at I2C or SPI.

Your controller driver itself is unaffected by this, you still call
snd_ac97_controller_register() from the probe function and so on.

> 
>>> +	void (*wait)(struct ac97_controller *adrv, int slot);
>>> +	void (*init)(struct ac97_controller *adrv, int slot);
>>
>> Neither wait nor init are ever used.
> This is because I've not begun to porting sound/pci/ac97_codec.c into
> sound/ac97.

Ok, makes sense. But maybe just leave them out for now and add them when
they are used.

[...]
>>> +	ret = sysfs_create_link(&codec->dev.kobj, &ac97_ctrl->dev->kobj,
>>> +				"ac97_controller");
>>
>> Since the CODEC is a child of the controller this should not be necessary as
>> this just points one directory up. It's like `ln -s .. parent`
> This creates :
> /sys/bus/ac97/devices/pxa2xx-ac97\:0/ac97_controller
> 
> Of course as you pointed out, it's a 'ln -s .. parent' like link, but it seems
> to me very unfriendly to have :
>  - /sys/bus/ac97/devices/pxa2xx-ac97\:0/../ac97_operations
>  - while /sys/bus/ac97/devices/ac97_operations doesn't exist (obviously)
> 
> Mmm, I don't know for this one, my mind is not set, it's a bit hard to tell if
> it's only an unecessary link bringing "comfort", or something usefull.

It is additional ABI and we do not have this for any other bus either (e.g.
you don't see a backlink for a I2C or SPI device to the parent). In my
opinion for the sake of keeping things consistent and simple we should not
add this.

>>
>>> +	if (ret)
>>> +		goto err_unregister_device;
>>> +
>>> +	return 0;
>>> +err_unregister_device:
>>> +	put_device(&codec->dev);
>>> +err_free_codec:
>>> +	kfree(codec);
>>
>> Since the struct is reference counted, the freeing is done in the release
>> callback and this leads to a double free.
> A yes in the "goto err_unregister_device" case right, while it's necessary in
> the "goto err_free_codec" case ... I need to rework that a bit.

It should use put_device() in both cases, check the the device_register()
documentation. It says that put_device() must be used, even if
device_register() fails.

> 
>>> +int snd_ac97_codec_driver_register(struct ac97_codec_driver *drv)
>>> +{
>>> +	int ret;
>>> +
>>> +	drv->driver.bus = &ac97_bus_type;
>>> +
>>> +	ret = driver_register(&drv->driver);
>>> +	if (!ret)
>>> +		ac97_rescan_all_controllers();
>>
>> Rescanning the bus when a new codec driver is registered should not be
>> neccessary. The bus is scanned once when the controller is registered, this
>> creates the device. The device driver core will take care of binding the
>> device to the driver, if the driver is registered after thed evice.
> That's because you suppose the initial scanning found all the ac97 codecs.
> But that's an incomplete vision as a codec might be powered off at that time and
> not seen by the first scanning, while the new scanning will discover it.

But why would the device become suddenly visible when the driver is
registered. This seems to be an as arbitrary point in time as any other.

Also consider that when you build a driver as a module, the module will
typically only be auto-loaded after the device has been detected, based on
the device ID. On the other hand, if the driver is built-in driver
registration will happen either before or shortly after the controller
registration.

If there is the expectation that the AC97 CODEC might randomly appear on the
bus, the core should periodically scan the bus.

[...]
>>> +	ac97_ctrl->ops->warm_reset(ac97_ctrl);
>>> +	return len;
>>> +}
>>> +static DEVICE_ATTR_WO(warm_reset);
>>> +
>>> +static struct attribute *ac97_controller_device_attrs[] = {
>>> +	&dev_attr_cold_reset.attr,
>>> +	&dev_attr_warm_reset.attr,
>>> +	NULL
>>> +};
>>
>> This adds new userspace ABI that is not documented at the moment.
> Very true. And as all userspace interface, it needs to be discussed. If nobody
> complains, I'll add the documentation for next patch round.
>>
>>> +int snd_ac97_controller_register(const struct ac97_controller_ops *ops,
>>> +				 struct device *dev,
>>> +				 unsigned short slots_available,
>>> +				 void **codecs_pdata)
>>
>> In my opinion this should return a handle to a ac97 controller which can
>> then be passed to snd_ac97_controller_unregister(). This is in my opinion
>> the better approach rather than looking up the controller by parent device.
> This is another "legacy drivers" issue.
> 
> The legacy driver (the one probed by platform_data or devicetree) doesn't
> necessarily have a private structure to store this ac97_controller pointer.

I might be missing something, but I'm not convinced by this. Can you point
me to such a legacy driver where you think this would not work?

[toc] | [prev] | [next] | [standalone]

#1518493

Lars-Peter Clausen <lars@metafoo.de> writes:

> On 11/08/2016 10:18 PM, Robert Jarzmik wrote:
>>> I'd make the controller itself a struct dev, rather than just having the
>>> pointer to the parent. This is more idiomatic and matches what other
>>> subsystems do. It has several advantages, you get proper refcounting of your
>>> controller structure, the controller gets its own sysfs directory where the
>>> CODECs appear as children, you don't need the manual sysfs attribute
>>> creation and removal in ac97_controler_{un,}register().
>> 
>> If you mean having "struct device dev" instead of "struct device *dev", it has
>> also a drawback : all the legacy platforms have already a probing method, be
>> that platform data, device-tree or something else.
>> 
>> I'm a bit converned about the conversion toll. Maybe I've not understood your
>> point fully, so please feel free to explain, with an actual example even better.
>
> This would be a struct device that is not bound to a driver, but just acts
> as a shell for the controller and places it inside the device hierarchy. You
> get reference counting and other management functions as well as a
> consistent naming scheme. E.g. you can call the devices ac97c%d (or
> something similar) and then call the CODEC ac97c%d.%d.
Let me think about it.
If I get you right, the device model would be, from a parenthood point of view:
             pxa2xx_ac97 (platform_device)
                  ^
                  |
             ac97_controller.dev (device, son of pxa2xx_ac97)
                 / \
                /   \
               /     \
              /       \
             /         \
         wm97xx-core  codec2     (sons ac97_controller.dev)

I have already the device hierarchy AFAIK, created in ac97_codec_add(). I'm
still failing to see the difference, as the refcounting is already used. The
only additional thing I see is the introduction of an intermediate
ac97_controller.dev which is refcounted.

In current model, pxa2xx_ac97 refcount is incremented for each codec device. In
the one you propose, pxa2xx_ac97 will be 1 refcounted (maybe 2 actually), and
ac97_controller.dev will be refcounted for each codec. This ac97_controller.dev
will be a spiritual twin of spi_master/i2c_adapter.

As I said, I must think about it and find which value is brought by this
additionnal layer.

As for the name change, I must check if this breaks the sound machine files, and
their dai_link structures (ex: mioa701_wm9713.c, structure mioa701_dai). In a
former state of this patchset I had changed the device names, ie. wm9713-codec
became pxa2xx-ac97.0, and the my machine file became broken.

> This is how most frameworks implementing some kind of control bus are
> structured in the Linux kernel. E.g. take a look at I2C or SPI.
I will.

>>>> +	ret = sysfs_create_link(&codec->dev.kobj, &ac97_ctrl->dev->kobj,
>>>> +				"ac97_controller");
>>>
>>> Since the CODEC is a child of the controller this should not be necessary as
>>> this just points one directory up. It's like `ln -s .. parent`
>> This creates :
>> /sys/bus/ac97/devices/pxa2xx-ac97\:0/ac97_controller
>> 
>> Of course as you pointed out, it's a 'ln -s .. parent' like link, but it seems
>> to me very unfriendly to have :
>>  - /sys/bus/ac97/devices/pxa2xx-ac97\:0/../ac97_operations
>>  - while /sys/bus/ac97/devices/ac97_operations doesn't exist (obviously)
>> 
>> Mmm, I don't know for this one, my mind is not set, it's a bit hard to tell if
>> it's only an unecessary link bringing "comfort", or something usefull.
>
> It is additional ABI and we do not have this for any other bus either (e.g.
> you don't see a backlink for a I2C or SPI device to the parent). In my
> opinion for the sake of keeping things consistent and simple we should not
> add this.
Fair enough.

>>>> +int snd_ac97_codec_driver_register(struct ac97_codec_driver *drv)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	drv->driver.bus = &ac97_bus_type;
>>>> +
>>>> +	ret = driver_register(&drv->driver);
>>>> +	if (!ret)
>>>> +		ac97_rescan_all_controllers();
>>>
>>> Rescanning the bus when a new codec driver is registered should not be
>>> neccessary. The bus is scanned once when the controller is registered, this
>>> creates the device. The device driver core will take care of binding the
>>> device to the driver, if the driver is registered after thed evice.
>> That's because you suppose the initial scanning found all the ac97 codecs.
>> But that's an incomplete vision as a codec might be powered off at that time and
>> not seen by the first scanning, while the new scanning will discover it.
>
> But why would the device become suddenly visible when the driver is
> registered. This seems to be an as arbitrary point in time as any other.
Because in the meantime, a regulator or something else provided power to the
AC97 IP, or a clock, and it can answer to requests after that.

And another point if that the clock of controller might be provided by the AC97
codec, and the scan will only succeed once the codec is actually providing this
clock, which it might do only once the module_init() is done.

> Also consider that when you build a driver as a module, the module will
> typically only be auto-loaded after the device has been detected, based on
> the device ID. On the other hand, if the driver is built-in driver
> registration will happen either before or shortly after the controller
> registration.
> If there is the expectation that the AC97 CODEC might randomly appear on the
> bus, the core should periodically scan the bus.
Power wise a periodical scan doesn't look good, at all.

More globally, I don't see if there is an actual issue we're trying to address
here, ie. that the rescan is a bug, or if it's more an "Occam's razor"
discussion ?

>>> In my opinion this should return a handle to a ac97 controller which can
>>> then be passed to snd_ac97_controller_unregister(). This is in my opinion
>>> the better approach rather than looking up the controller by parent device.
>> This is another "legacy drivers" issue.
>> 
>> The legacy driver (the one probed by platform_data or devicetree) doesn't
>> necessarily have a private structure to store this ac97_controller pointer.
>
> I might be missing something, but I'm not convinced by this. Can you point
> me to such a legacy driver where you think this would not work?
The first one that popped out:
 - hac_soc_platform_probe()

Cheers.

-- 
Robert

[toc] | [prev] | [next] | [standalone]

#1518891

On 11/09/2016 10:27 PM, Robert Jarzmik wrote:
[...]
>>>>> +int snd_ac97_codec_driver_register(struct ac97_codec_driver *drv)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	drv->driver.bus = &ac97_bus_type;
>>>>> +
>>>>> +	ret = driver_register(&drv->driver);
>>>>> +	if (!ret)
>>>>> +		ac97_rescan_all_controllers();
>>>>
>>>> Rescanning the bus when a new codec driver is registered should not be
>>>> neccessary. The bus is scanned once when the controller is registered, this
>>>> creates the device. The device driver core will take care of binding the
>>>> device to the driver, if the driver is registered after thed evice.
>>> That's because you suppose the initial scanning found all the ac97 codecs.
>>> But that's an incomplete vision as a codec might be powered off at that time and
>>> not seen by the first scanning, while the new scanning will discover it.
>>
>> But why would the device become suddenly visible when the driver is
>> registered. This seems to be an as arbitrary point in time as any other.
> Because in the meantime, a regulator or something else provided power to the
> AC97 IP, or a clock, and it can answer to requests after that.
> 
> And another point if that the clock of controller might be provided by the AC97
> codec, and the scan will only succeed once the codec is actually providing this
> clock, which it might do only once the module_init() is done.
> 
>> Also consider that when you build a driver as a module, the module will
>> typically only be auto-loaded after the device has been detected, based on
>> the device ID. On the other hand, if the driver is built-in driver
>> registration will happen either before or shortly after the controller
>> registration.
>> If there is the expectation that the AC97 CODEC might randomly appear on the
>> bus, the core should periodically scan the bus.
> Power wise a periodical scan doesn't look good, at all.
> 
> More globally, I don't see if there is an actual issue we're trying to address
> here, ie. that the rescan is a bug, or if it's more an "Occam's razor"
> discussion ?

It's a framework design discussion. In my opinion the driver being
registered and the device becoming visible on the physical bus are two
completely unrelated operations. Especially in the Linux device driver model.

You have a generic driver that calls ac97_codec_driver_register() in its
module_init() section. This generic driver does (at module_init() time) not
know anything about a device instance specific clocks, regulators or other
resources. Resources are handled on a per device instance basis, and you
won't have a device instance until the device has been detected, which
happens in ac97_bus_scan(). So you have a cyclic dependency loop here.

Maybe we can just leave the rescanning out for now and think about how to
best handle it when the need arises.

> 
>>>> In my opinion this should return a handle to a ac97 controller which can
>>>> then be passed to snd_ac97_controller_unregister(). This is in my opinion
>>>> the better approach rather than looking up the controller by parent device.
>>> This is another "legacy drivers" issue.
>>>
>>> The legacy driver (the one probed by platform_data or devicetree) doesn't
>>> necessarily have a private structure to store this ac97_controller pointer.
>>
>> I might be missing something, but I'm not convinced by this. Can you point
>> me to such a legacy driver where you think this would not work?
> The first one that popped out:
>  - hac_soc_platform_probe()

I think that driver should be able to use platform_set_drvdata() to store
the pointer.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web