Re: [PATCH v2] drivers: most: add ALSA sound driver
On Tue, 2020-11-17 at 11:41 +0300, Dan Carpenter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Tue, Nov 17, 2020 at 08:08:50AM +, christian.gr...@microchip.com wrote:
> > On Tue, 2020-11-10 at 11:48 +0300, Dan Carpenter wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > > the content is safe
> > >
> > > On Fri, Nov 06, 2020 at 05:30:54PM +0100, Christian Gromm wrote:
> > > > +static struct list_head adpt_list;
> > > > +
> > > > +#define MOST_PCM_INFO (SNDRV_PCM_INFO_MMAP | \
> > > > +SNDRV_PCM_INFO_MMAP_VALID | \
> > > > +SNDRV_PCM_INFO_BATCH | \
> > > > +SNDRV_PCM_INFO_INTERLEAVED | \
> > > > +SNDRV_PCM_INFO_BLOCK_TRANSFER)
> > > > +
> > > > +static void swap_copy16(u16 *dest, const u16 *source, unsigned int
> > > > bytes)
> > > > +{
> > > > + unsigned int i = 0;
> > > > +
> > > > + while (i < (bytes / 2)) {
> > > > + dest[i] = swab16(source[i]);
> > > > + i++;
> > > > + }
> > > > +}
> > > > +
> > > > +static void swap_copy24(u8 *dest, const u8 *source, unsigned int bytes)
> > > > +{
> > > > + unsigned int i = 0;
> > > > +
> > > > + while (i < bytes - 2) {
> > >
> > > Can bytes ever be zero? If so then this will corrupt memory and crash.
> > >
> > > Generally "int i;" is less risky than "unsigned int i;". Of course, I
> > > recently almost introduced a situation where we were copying up to
> > > ULONG_MAX bytes so there are times when iterators should be size_t so
> > > that does happen. It could be buggy either way is what I'm saying but
> > > generally "unsigned int i;" is more often buggy.
> > >
> > > > + dest[i] = source[i + 2];
> > > > + dest[i + 1] = source[i + 1];
> > > > + dest[i + 2] = source[i];
> > > > + i += 3;
> > > > + }
> > > > +}
> > > > +
> > > > +static void swap_copy32(u32 *dest, const u32 *source, unsigned int
> > > > bytes)
> > > > +{
> > > > + unsigned int i = 0;
> > > > +
> > > > + while (i < bytes / 4) {
> > > > + dest[i] = swab32(source[i]);
> > > > + i++;
> > > > + }
> > > > +}
> > > > +
> > > > +static void alsa_to_most_memcpy(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + memcpy(most, alsa, bytes);
> > > > +}
> > > > +
> > > > +static void alsa_to_most_copy16(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + swap_copy16(most, alsa, bytes);
> > > > +}
> > > > +
> > > > +static void alsa_to_most_copy24(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + swap_copy24(most, alsa, bytes);
> > > > +}
> > > > +
> > > > +static void alsa_to_most_copy32(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + swap_copy32(most, alsa, bytes);
> > > > +}
> > > > +
> > > > +static void most_to_alsa_memcpy(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + memcpy(alsa, most, bytes);
> > > > +}
> > > > +
> > > > +static void most_to_alsa_copy16(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + swap_copy16(alsa, most, bytes);
> > > > +}
> > > > +
> > > > +static void most_to_alsa_copy24(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + swap_copy24(alsa, most, bytes);
> > > > +}
> > > > +
> > > > +static void most_to_alsa_copy32(void *alsa, void *most, unsigned int
> > > > bytes)
> > > > +{
> > > > + swap_copy32(alsa, most, bytes);
> > > > +}
> > > > +
> > > > +/**
> > > > + * get_channel - get pointer to channel
> > > > + * @iface: interface structure
> > > > + * @channel_id: channel ID
> > > > + *
> > > > + * This traverses the channel list and returns the channel matching the
> > > > + * ID and interface.
> > > > + *
> > > > + * Returns pointer to channel on success or NULL otherwise.
> > > > + */
> > > > +static struct channel *get_channel(struct most_interface *iface,
> > > > +int channel_id)
> > > > +{
> > > > + struct sound_adapter *adpt = iface->priv;
> > > > + struct channel *channel, *tmp;
> > > > +
> > > > + list_for_each_entry_safe(channel, tmp, >dev_list, list) {
> > > > + if ((channel->iface == iface) && (channel->id ==
> > > > channel_id))
> > > > + return channel;
> > >
> > > No need to use the _safe() version of this loop macro. You're not
> > > freeing anything. My concern is that sometimes people think the _safe()
> > > has something to do with locking and it does not.
> > >
> > > > + }
> > > > + return NULL;
> > > > +}
> > > > +
> > >
> > > [ Snip ]
> > >
> > > > +/**
> > > > + * audio_probe_channel - probe function of the driver module
> > > > + * @iface: pointer to interface instance
> > > > + * @channel_id: channel index/ID
> > > > + * @cfg: pointer to actual channel
Re: [PATCH v2] drivers: most: add ALSA sound driver
On Tue, 2020-11-10 at 11:48 +0300, Dan Carpenter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Fri, Nov 06, 2020 at 05:30:54PM +0100, Christian Gromm wrote:
> > +static struct list_head adpt_list;
> > +
> > +#define MOST_PCM_INFO (SNDRV_PCM_INFO_MMAP | \
> > +SNDRV_PCM_INFO_MMAP_VALID | \
> > +SNDRV_PCM_INFO_BATCH | \
> > +SNDRV_PCM_INFO_INTERLEAVED | \
> > +SNDRV_PCM_INFO_BLOCK_TRANSFER)
> > +
> > +static void swap_copy16(u16 *dest, const u16 *source, unsigned int bytes)
> > +{
> > + unsigned int i = 0;
> > +
> > + while (i < (bytes / 2)) {
> > + dest[i] = swab16(source[i]);
> > + i++;
> > + }
> > +}
> > +
> > +static void swap_copy24(u8 *dest, const u8 *source, unsigned int bytes)
> > +{
> > + unsigned int i = 0;
> > +
> > + while (i < bytes - 2) {
>
> Can bytes ever be zero? If so then this will corrupt memory and crash.
>
> Generally "int i;" is less risky than "unsigned int i;". Of course, I
> recently almost introduced a situation where we were copying up to
> ULONG_MAX bytes so there are times when iterators should be size_t so
> that does happen. It could be buggy either way is what I'm saying but
> generally "unsigned int i;" is more often buggy.
>
> > + dest[i] = source[i + 2];
> > + dest[i + 1] = source[i + 1];
> > + dest[i + 2] = source[i];
> > + i += 3;
> > + }
> > +}
> > +
> > +static void swap_copy32(u32 *dest, const u32 *source, unsigned int bytes)
> > +{
> > + unsigned int i = 0;
> > +
> > + while (i < bytes / 4) {
> > + dest[i] = swab32(source[i]);
> > + i++;
> > + }
> > +}
> > +
> > +static void alsa_to_most_memcpy(void *alsa, void *most, unsigned int bytes)
> > +{
> > + memcpy(most, alsa, bytes);
> > +}
> > +
> > +static void alsa_to_most_copy16(void *alsa, void *most, unsigned int bytes)
> > +{
> > + swap_copy16(most, alsa, bytes);
> > +}
> > +
> > +static void alsa_to_most_copy24(void *alsa, void *most, unsigned int bytes)
> > +{
> > + swap_copy24(most, alsa, bytes);
> > +}
> > +
> > +static void alsa_to_most_copy32(void *alsa, void *most, unsigned int bytes)
> > +{
> > + swap_copy32(most, alsa, bytes);
> > +}
> > +
> > +static void most_to_alsa_memcpy(void *alsa, void *most, unsigned int bytes)
> > +{
> > + memcpy(alsa, most, bytes);
> > +}
> > +
> > +static void most_to_alsa_copy16(void *alsa, void *most, unsigned int bytes)
> > +{
> > + swap_copy16(alsa, most, bytes);
> > +}
> > +
> > +static void most_to_alsa_copy24(void *alsa, void *most, unsigned int bytes)
> > +{
> > + swap_copy24(alsa, most, bytes);
> > +}
> > +
> > +static void most_to_alsa_copy32(void *alsa, void *most, unsigned int bytes)
> > +{
> > + swap_copy32(alsa, most, bytes);
> > +}
> > +
> > +/**
> > + * get_channel - get pointer to channel
> > + * @iface: interface structure
> > + * @channel_id: channel ID
> > + *
> > + * This traverses the channel list and returns the channel matching the
> > + * ID and interface.
> > + *
> > + * Returns pointer to channel on success or NULL otherwise.
> > + */
> > +static struct channel *get_channel(struct most_interface *iface,
> > +int channel_id)
> > +{
> > + struct sound_adapter *adpt = iface->priv;
> > + struct channel *channel, *tmp;
> > +
> > + list_for_each_entry_safe(channel, tmp, >dev_list, list) {
> > + if ((channel->iface == iface) && (channel->id == channel_id))
> > + return channel;
>
> No need to use the _safe() version of this loop macro. You're not
> freeing anything. My concern is that sometimes people think the _safe()
> has something to do with locking and it does not.
>
> > + }
> > + return NULL;
> > +}
> > +
>
> [ Snip ]
>
> > +/**
> > + * audio_probe_channel - probe function of the driver module
> > + * @iface: pointer to interface instance
> > + * @channel_id: channel index/ID
> > + * @cfg: pointer to actual channel configuration
> > + * @arg_list: string that provides the name of the device to be created in
> > /dev
> > + * plus the desired audio resolution
> > + *
> > + * Creates sound card, pcm device, sets pcm ops and registers sound card.
> > + *
> > + * Returns 0 on success or error code otherwise.
> > + */
> > +static int audio_probe_channel(struct most_interface *iface, int
> > channel_id,
> > +struct most_channel_config *cfg,
> > +char *device_name, char *arg_list)
> > +{
> > + struct channel *channel;
> > + struct sound_adapter *adpt;
> > + struct snd_pcm *pcm;
> > + int playback_count = 0;
> > + int capture_count = 0;
> > + int ret;
> > + int direction;
> > + u16 ch_num;
> > + char *sample_res;
> > + char arg_list_cpy[STRING_SIZE];
> > +
> > +
Re: [PATCH] drivers: most: add ALSA sound driver
On Mon, 2020-11-02 at 16:31 +0100, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Mon, Nov 02, 2020 at 04:14:03PM +0100, Christian Gromm wrote:
> > This patch moves the ALSA sound driver out of the staging area and
> > adds it
> > to the stable part of the MOST driver. Modifications to the
> > Makefiles and
> > Kconfigs are done accordingly to not break the build.
> >
> > Signed-off-by: Christian Gromm
> > ---
> > drivers/most/Kconfig| 10 +
> > drivers/most/Makefile | 1 +
> > drivers/most/most_snd.c | 753
> >
> > drivers/staging/most/Kconfig| 2 -
> > drivers/staging/most/Makefile | 1 -
> > drivers/staging/most/sound/Kconfig | 14 -
> > drivers/staging/most/sound/Makefile | 4 -
> > drivers/staging/most/sound/sound.c | 753
> >
> > 8 files changed, 764 insertions(+), 774 deletions(-)
> > create mode 100644 drivers/most/most_snd.c
> > delete mode 100644 drivers/staging/most/sound/Kconfig
> > delete mode 100644 drivers/staging/most/sound/Makefile
> > delete mode 100644 drivers/staging/most/sound/sound.c
> >
> > diff --git a/drivers/most/Kconfig b/drivers/most/Kconfig
> > index ebfe84e..4b8145b 100644
> > --- a/drivers/most/Kconfig
> > +++ b/drivers/most/Kconfig
> > @@ -32,4 +32,14 @@ config MOST_CDEV
> >
> > To compile this driver as a module, choose M here: the
> > module will be called most_cdev.
> > +
> > +config MOST_SND
> > + tristate "Sound"
> > + depends on SND
> > + select SND_PCM
> > + help
> > + Say Y here if you want to commumicate via ALSA/sound
> > devices.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called most_sound.
> > endif
> > diff --git a/drivers/most/Makefile b/drivers/most/Makefile
> > index 8b53ca4..60db6cd 100644
> > --- a/drivers/most/Makefile
> > +++ b/drivers/most/Makefile
> > @@ -5,3 +5,4 @@ most_core-y :=core.o \
> >
> > obj-$(CONFIG_MOST_USB_HDM) += most_usb.o
> > obj-$(CONFIG_MOST_CDEV) += most_cdev.o
> > +obj-$(CONFIG_MOST_SND) += most_snd.o
> > diff --git a/drivers/most/most_snd.c b/drivers/most/most_snd.c
> > new file mode 100644
> > index 000..8a449ab
> > --- /dev/null
> > +++ b/drivers/most/most_snd.c
> > @@ -0,0 +1,753 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * sound.c - Sound component for Mostcore
> > + *
> > + * Copyright (C) 2015 Microchip Technology Germany II GmbH & Co.
> > KG
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define DRIVER_NAME "sound"
> > +#define STRING_SIZE 80
> > +
> > +static struct most_component comp;
> > +
> > +/**
> > + * struct channel - private structure to keep channel specific
> > data
> > + * @substream: stores the substream structure
> > + * @iface: interface for which the channel belongs to
> > + * @cfg: channel configuration
> > + * @card: registered sound card
> > + * @list: list for private use
> > + * @id: channel index
> > + * @period_pos: current period position (ring buffer)
> > + * @buffer_pos: current buffer position (ring buffer)
> > + * @is_stream_running: identifies whether a stream is running or
> > not
> > + * @opened: set when the stream is opened
> > + * @playback_task: playback thread
> > + * @playback_waitq: waitq used by playback thread
> > + */
> > +struct channel {
> > + struct snd_pcm_substream *substream;
> > + struct snd_pcm_hardware pcm_hardware;
> > + struct most_interface *iface;
> > + struct most_channel_config *cfg;
> > + struct snd_card *card;
> > + struct list_head list;
> > + int id;
> > + unsigned int period_pos;
> > + unsigned int buffer_pos;
> > + bool is_stream_running;
> > + struct task_struct *playback_task;
> > + wait_queue_head_t playback_waitq;
> > + void (*copy_fn)(void *alsa, void *most, unsigned int bytes);
> > +};
> > +
> > +struct sound_adapter {
> > + struct list_head dev_list;
> > + struct most_interface *iface;
> > + struct snd_card *card;
> > + struct list_head list;
> > + bool registered;
> > + int pcm_dev_idx;
> > +};
> > +
> > +static struct list_head adpt_list;
> > +
> > +#define MOST_PCM_INFO (SNDRV_PCM_INFO_MMAP | \
> > +SNDRV_PCM_INFO_MMAP_VALID | \
> > +SNDRV_PCM_INFO_BATCH | \
> > +SNDRV_PCM_INFO_INTERLEAVED | \
> > +SNDRV_PCM_INFO_BLOCK_TRANSFER)
> > +
> > +#define swap16(val) ( \
> > + (((u16)(val) << 8) & (u16)0xFF00) | \
> > + (((u16)(val) >> 8) & (u16)0x00FF))
> > +
> > +#define swap32(val) ( \
> > + (((u32)(val) << 24) & (u32)0xFF00) | \
> > +
Re: [PATCH] staging: most: usb: rename most_usb.ko
On Wed, 2020-07-29 at 19:03 +0200, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Wed, Jul 29, 2020 at 06:38:48PM +0200, Christian Gromm wrote:
> > To avoid a name conflict when adding the usb module to the
> > driver's directory in the stable branch, this patch simply
> > renames the kernel object.
> >
> > Signed-off-by: Christian Gromm
> > Reported-by: Greg Kroah-Hartman
> > ---
> > drivers/staging/most/usb/{most_usb.ko => most-usb.ko} | Bin
> > 1 file changed, 0 insertions(+), 0 deletions(-)
> > rename drivers/staging/most/usb/{most_usb.ko => most-usb.ko}
> > (100%)
> >
> > diff --git a/drivers/staging/most/usb/most_usb.ko
> > b/drivers/staging/most/usb/most-usb.ko
> > similarity index 100%
> > rename from drivers/staging/most/usb/most_usb.ko
> > rename to drivers/staging/most/usb/most-usb.ko
>
> You renamed a binary file??? That is not in the source tree?
>
I know. And I was kind of confused that you chose this path (1).
I even had to mess up my git to do that.
>
> No, I mean make the patch move the .c file from staging to the
> drivers/most directory and adjust the Kconfig/Makefiles for that
> movement.
>
Huh, but this is exactly what I wanted to do in the first place.
Add it to the stable branch and change the staging files to
avoid the conflict.
But then you told me to not touch the staging files. Remember?
Anyways, here is what I am going to do now:
add the usb file to the stable branch, change the name of the
.ko inside the stable branch and then once the staging files
are removed, I'll rename it again to get the old name back.
Does this make sense now?
thanks,
Chris
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RESEND PATCH v5] drivers: most: add USB adapter driver
On Wed, 2020-07-29 at 16:27 +0200, Greg KH wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Mon, Jul 27, 2020 at 10:30:46AM +0200, Christian Gromm wrote: > > This patch adds the usb driver source file most_usb.c and > > modifies the Makefile and Kconfig accordingly. > > > > Signed-off-by: Christian Gromm > > Sorry for the long delay in getting to this. > > This looks good to me, but I tried to apply it to my usb tree, and of > course I get the following build error: > error: the following would cause module name conflict: > drivers/staging/most/usb/most_usb.ko > drivers/most/most_usb.ko > > So, can you just send a "rename the file" patch that I can apply > against > my staging-next branch and I will take it through there so that there > are no conflicts like this? Are you talking about (1) a patch that just renames the most_usb.ko file located at drivers/staging/most/usb/ or (2) a patch for the Makefile in the staging tree, so the Kbuild system creates a new kernel object in this branch with a different name? thank, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RESEND PATCH v5] drivers: most: add USB adapter driver
On Mon, 2020-07-27 at 07:59 -0700, Randy Dunlap wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > Hi-- > > On 7/27/20 1:30 AM, Christian Gromm wrote: > > This patch adds the usb driver source file most_usb.c and > > modifies the Makefile and Kconfig accordingly. > > > > Signed-off-by: Christian Gromm > > --- > > > > drivers/most/Kconfig| 12 + > > drivers/most/Makefile |2 + > > drivers/most/most_usb.c | 1170 > > +++ > > 3 files changed, 1184 insertions(+) > > create mode 100644 drivers/most/most_usb.c > > > > diff --git a/drivers/most/Kconfig b/drivers/most/Kconfig > > index 58d7999..7b65320 100644 > > --- a/drivers/most/Kconfig > > +++ b/drivers/most/Kconfig > > @@ -13,3 +13,15 @@ menuconfig MOST > > module will be called most_core. > > > > If in doubt, say N here. > > + > > +if MOST > > +config MOST_USB_HDM > > + tristate "USB" > > + depends on USB && NET > > + help > > + Say Y here if you want to connect via USB to network > > transceiver. > > + This device depends on the networking AIM. > > What does that last sentence above mean? Nothing. This dependency is obsolete and needs to be removed. I'll fix this up. thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/8] drivers: most: add usb adapter driver
On Wed, 2020-05-20 at 16:17 +0300, Dan Carpenter wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Thu, May 14, 2020 at 11:52:49AM +0200, Christian Gromm wrote: > > This patch adds the usb driver source file most_usb.c and > > modifies the Makefile and Kconfig accordingly. > > > > Signed-off-by: Christian Gromm > > --- > > v2: > > Reported-by: Greg Kroah-Hartman > > - don't remove usb driver from staging area > > - don't touch staging/most/Kconfig > > - remove subdirectory for USB driver and put source file into > > drivers/most > > > > drivers/most/Kconfig| 12 + > > drivers/most/Makefile |2 + > > drivers/most/most_usb.c | 1262 > > +++ > > 3 files changed, 1276 insertions(+) > > create mode 100644 drivers/most/most_usb.c > > > > There is already a v3 out there that has some of your concerns already addressed. I'll take your comments and fix them in the next version of the patch. thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/7] staging: most: usb: check number of reported endpoints
On Thu, 2020-05-14 at 16:06 +0200, Greg KH wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Thu, May 14, 2020 at 03:46:25PM +0200, Christian Gromm wrote: > > This patch checks the number of endpoints reported by the USB > > interface descriptor and throws an error if the number exceeds > > MAX_NUM_ENDPOINTS. > > > > Signed-off-by: Christian Gromm > > Reported-by: Greg Kroah-Hartman > > --- > > drivers/staging/most/usb/usb.c | 13 ++--- > > 1 file changed, 6 insertions(+), 7 deletions(-) > > This is a bugfix that should get backported to stable kernels, right? > Right. I already sent out a v2 of the series. Should I resend? thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC] drivers: most: add USB adapter driver
On Mon, 2020-05-11 at 18:33 +0200, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Mon, May 11, 2020 at 02:46:58PM +,
> christian.gr...@microchip.com wrote:
> > On Mon, 2020-05-11 at 13:47 +0200, Greg KH wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > >
> > > On Mon, May 11, 2020 at 11:51:15AM +0200, Christian Gromm wrote:
> > > > This patch adds the MOST USB adapter driver to the stable
> > > > branch.
> > > > This is
> > > > a follow-up to commit .
> > >
> > > I do not understand the "a follow-up..." sentance. Always use
> > > the
> > > format of:
> > > b27652753918 ("staging: most: move core files out of the
> > > staging area")
> > > when writing kernel commits in changelogs.
> > >
> > > Also, that commit doesn't really mean anything here, this is a
> > > stand-alone driver for the most subsystem. This changelog needs
> > > work.
> >
> > Purpose was sharing the information that this is patch is
> > only one part of moving the complete driver stack. That a
> > first step has alread been done and others are to follow.
> > But you're probably right and nobody realy needs to know.
> >
> > I'll skip this.
> >
> > > > Signed-off-by: Christian Gromm
> > > > ---
> > > > drivers/most/Kconfig |6 +
> > > > drivers/most/Makefile |2 +
> > > > drivers/most/usb/Kconfig | 14 +
> > > > drivers/most/usb/Makefile |4 +
> > > > drivers/most/usb/usb.c| 1262
> > > > +
> > >
> > > Why not just call this file most-usb.c so you don't have to do
> > > the
> > > 2-step Makefile work. Also, why a whole subdir for a single .c
> > > file?
> >
> > To keep the staging layout.
>
> No need to do that, this is a new layout :)
>
> > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > >
> > > You shouldn't need any pr_*() calls because this is a driver and
> > > you
> > > always have access to the struct device * it controls. So drop
> > > this
> > > and
> > > fix up the remaining pr_*() calls to be dev_*() instead.
> >
> > There are helper functions that actually don't have access to the
> > struct device and it felt like an overhead to pass the device
> > pointer just for logging purposes.
>
> pr_* calls show almost nothing when it comes to the actual
> device/driver
> being affected. That's why the dev_*() functions are there, please
> use
> them.
>
> > > > +/**
> > > > + * struct most_dci_obj - Direct Communication Interface
> > > > + * @kobj:position in sysfs
> > > > + * @usb_device: pointer to the usb device
> > > > + * @reg_addr: register address for arbitrary DCI access
> > > > + */
> > > > +struct most_dci_obj {
> > > > + struct device dev;
> > >
> > > Wait, why is a USB driver creating something with a separate
> > > struct
> > > device embedded in it? Shouldn't the most core handle stuff like
> > > this?
> >
> > The driver adds an ABI interface that belongs to USB only. This
> > keeps
> > the core generic.
>
> So this same type of thing is also needed in the other bus
> controllers
> (serial, i2c, etc.)?
>
> Creating a new device implies it lives on a bus, and almost always
> the
> bus code for creating/managing that code lives in a single place, not
> in
> the individual drivers. Why doesn't the most core handle this? What
> does the most core do? :)
The core module manages the buffers, routing, configuration,
sysfs/configfs and user space interface (via its component modules)
for common communication channels. The DCI interface is only available
when the hardware is connected via USB. Other connections do not
provide this.
That's why I want the module that actually introduces such an
interface (and has all the necessary information about it) to be in
charge. This keeps the core code simpler, as USB isn't always used.
Also, a new device is needed to create the desired sysfs layout,
in which the dci interface is represented with a new sub directory.
thanks,
Chris
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC] drivers: most: add USB adapter driver
On Mon, 2020-05-11 at 13:47 +0200, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Mon, May 11, 2020 at 11:51:15AM +0200, Christian Gromm wrote:
> > This patch adds the MOST USB adapter driver to the stable branch.
> > This is
> > a follow-up to commit .
>
> I do not understand the "a follow-up..." sentance. Always use the
> format of:
> b27652753918 ("staging: most: move core files out of the
> staging area")
> when writing kernel commits in changelogs.
>
> Also, that commit doesn't really mean anything here, this is a
> stand-alone driver for the most subsystem. This changelog needs
> work.
Purpose was sharing the information that this is patch is
only one part of moving the complete driver stack. That a
first step has alread been done and others are to follow.
But you're probably right and nobody realy needs to know.
I'll skip this.
>
> > Signed-off-by: Christian Gromm
> > ---
> > drivers/most/Kconfig |6 +
> > drivers/most/Makefile |2 +
> > drivers/most/usb/Kconfig | 14 +
> > drivers/most/usb/Makefile |4 +
> > drivers/most/usb/usb.c| 1262
> > +
>
> Why not just call this file most-usb.c so you don't have to do the
> 2-step Makefile work. Also, why a whole subdir for a single .c file?
To keep the staging layout.
>
> > drivers/staging/most/Kconfig |2 -
> > drivers/staging/most/Makefile |1 -
>
> Why touch the staging directory for this patch? We can delete the
> old
> driver after this one is merged, no need for that here.
>
> > diff --git a/drivers/most/usb/usb.c b/drivers/most/usb/usb.c
> > new file mode 100644
> > index 000..daa5e4b
> > --- /dev/null
> > +++ b/drivers/most/usb/usb.c
> > @@ -0,0 +1,1262 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * usb.c - Hardware dependent module for USB
> > + *
> > + * Copyright (C) 2013-2015 Microchip Technology Germany II GmbH &
> > Co. KG
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> You shouldn't need any pr_*() calls because this is a driver and you
> always have access to the struct device * it controls. So drop this
> and
> fix up the remaining pr_*() calls to be dev_*() instead.
There are helper functions that actually don't have access to the
struct device and it felt like an overhead to pass the device
pointer just for logging purposes.
>
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define USB_MTU 512
> > +#define NO_ISOCHRONOUS_URB 0
> > +#define AV_PACKETS_PER_XACT 2
> > +#define BUF_CHAIN_SIZE 0x
> > +#define MAX_NUM_ENDPOINTS30
> > +#define MAX_SUFFIX_LEN 10
> > +#define MAX_STRING_LEN 80
> > +#define MAX_BUF_SIZE 0x
> > +
> > +#define USB_VENDOR_ID_SMSC 0x0424 /* VID: SMSC */
> > +#define USB_DEV_ID_BRDG 0xC001 /* PID: USB Bridge */
> > +#define USB_DEV_ID_OS81118 0xCF18 /* PID: USB OS81118 */
> > +#define USB_DEV_ID_OS81119 0xCF19 /* PID: USB OS81119 */
> > +#define USB_DEV_ID_OS81210 0xCF30 /* PID: USB OS81210 */
> > +/* DRCI Addresses */
> > +#define DRCI_REG_NI_STATE0x0100
> > +#define DRCI_REG_PACKET_BW 0x0101
> > +#define DRCI_REG_NODE_ADDR 0x0102
> > +#define DRCI_REG_NODE_POS0x0103
> > +#define DRCI_REG_MEP_FILTER 0x0140
> > +#define DRCI_REG_HASH_TBL0 0x0141
> > +#define DRCI_REG_HASH_TBL1 0x0142
> > +#define DRCI_REG_HASH_TBL2 0x0143
> > +#define DRCI_REG_HASH_TBL3 0x0144
> > +#define DRCI_REG_HW_ADDR_HI 0x0145
> > +#define DRCI_REG_HW_ADDR_MI 0x0146
> > +#define DRCI_REG_HW_ADDR_LO 0x0147
> > +#define DRCI_REG_BASE0x1100
> > +#define DRCI_COMMAND 0x02
> > +#define DRCI_READ_REQ0xA0
> > +#define DRCI_WRITE_REQ 0xA1
> > +
> > +/**
> > + * struct most_dci_obj - Direct Communication Interface
> > + * @kobj:position in sysfs
> > + * @usb_device: pointer to the usb device
> > + * @reg_addr: register address for arbitrary DCI access
> > + */
> > +struct most_dci_obj {
> > + struct device dev;
>
> Wait, why is a USB driver creating something with a separate struct
> device embedded in it? Shouldn't the most core handle stuff like
> this?
The driver adds an ABI interface that belongs to USB only. This keeps
the core generic.
>
> > + struct usb_device *usb_device;
> > + u16 reg_addr;
> > +};
> > +
> > +#define to_dci_obj(p) container_of(p, struct most_dci_obj, dev)
> > +
> > +struct most_dev;
>
> Don't you already have this in the most.h file?
No. This belongs to USB only and must not be published with
the most.h header.
>
> > +
> > +struct clear_hold_work {
> > +
Re: [PATCH] staging: most: usb: fix exception handling
On Mon, 2020-05-04 at 17:39 +0200, Greg KH wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Mon, May 04, 2020 at 03:17:53PM +, > christian.gr...@microchip.com wrote: > > On Mon, 2020-05-04 at 15:54 +0200, Greg KH wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > > know the content is safe > > > > > > On Mon, May 04, 2020 at 03:44:00PM +0200, Christian Gromm wrote: > > > > This patch fixes error handling on function parameters. > > > > > > What does that mean? If I don't understand it, I think it needs > > > to > > > be > > > made a lot more explicit as to why you are making these changes > > > :) > > > > > > > Signed-off-by: Christian Gromm > > > > > > Any "Fixes:" tag for this? > > > > No. Just wanted to fix some obvious things, before adding > > it to stable, as discussed in our last email thread. > > Remember, no one has context when reading a git commit log, please > spell > it out :) > > > > SHould it go to stable if it really resolves issues? > > > > No. Once you accept this, I'll add it to stable anyway. > > How? Put the cc: stable on a patch if it fixes a real bug. I don't > see > what this "fixes" still... The interface pointer provided as a function parameter has already been used before it has been checked against NULL. :( Once I have the unnecessary parameter checking removed, the problem will be removed too. Can you please drop this patch and I'll send a new one. V2 does not make very much sense, as the patch will be doing somethig differnt now. thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: most: usb: fix exception handling
On Mon, 2020-05-04 at 15:55 +0200, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Mon, May 04, 2020 at 03:44:00PM +0200, Christian Gromm wrote:
> > This patch fixes error handling on function parameters.
> >
> > Signed-off-by: Christian Gromm
> > ---
> > drivers/staging/most/usb/usb.c | 33 +-
> > ---
> > 1 file changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/staging/most/usb/usb.c
> > b/drivers/staging/most/usb/usb.c
> > index e8c5a8c..e5276524 100644
> > --- a/drivers/staging/most/usb/usb.c
> > +++ b/drivers/staging/most/usb/usb.c
> > @@ -229,14 +229,14 @@ static unsigned int
> > get_stream_frame_size(struct most_channel_config *cfg)
> > */
> > static int hdm_poison_channel(struct most_interface *iface, int
> > channel)
> > {
> > - struct most_dev *mdev = to_mdev(iface);
> > + struct most_dev *mdev;
> > unsigned long flags;
> > spinlock_t *lock; /* temp. lock */
> >
> > if (unlikely(!iface)) {
> > - dev_warn(>usb_device->dev, "Poison: Bad
> > interface.\n");
> > - return -EIO;
> > + return -EFAULT;
>
> -EFAULT is ONLY for when you have an error with copying memory
> to/from
> userspace.
Ok.
>
> This should just be -EINVAL, right?
>
> And how can iface ever be NULL?
It should never become NULL. But you never know, right?
Too paranoid?
>
> And why unlikely() there, can you measure the difference with/without
> it? If not, please drop as the compiler/CPU can do it faster than
> you
> ever can.
>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: most: usb: fix exception handling
On Mon, 2020-05-04 at 15:54 +0200, Greg KH wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Mon, May 04, 2020 at 03:44:00PM +0200, Christian Gromm wrote: > > This patch fixes error handling on function parameters. > > What does that mean? If I don't understand it, I think it needs to > be > made a lot more explicit as to why you are making these changes :) > > > Signed-off-by: Christian Gromm > > Any "Fixes:" tag for this? No. Just wanted to fix some obvious things, before adding it to stable, as discussed in our last email thread. > > SHould it go to stable if it really resolves issues? No. Once you accept this, I'll add it to stable anyway. thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC] staging: most: usb: move USB adapter driver to stable branch
On Thu, 2020-04-30 at 10:13 +0200, Greg KH wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Thu, Apr 30, 2020 at 10:09:31AM +0200, Greg KH wrote: > > On Thu, Apr 30, 2020 at 10:07:35AM +0200, Greg KH wrote: > > > On Thu, Apr 30, 2020 at 10:00:12AM +0200, Christian Gromm wrote: > > > > This patch moves the MOST USB adapter driver to the stable > > > > branch. > > > > It is a follow-up to commit . > > > > > > That's not how to refer to git commit ids. Please see the > > > submitting-patches.rst file for the proper format. > > > > Also, you might want to run this driver by the usb mailing list, So I just put linux-...@vger.kernel.org on cc? > > I don't > > think it's been audited by anyone, myself included, for any usb > > specific > > things. Probably not. > > > > As proof, I see at least one thing that should be changed... > > Ok, lots of little things, I don't think I ever actually read this > driver before, sorry about that... > > Please post it as a patch that just adds the file, And another patch that removes the one from the staging directory? thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: b276527539 ("staging: most: move core files out of the staging .."): [ 12.247349] BUG: kernel NULL pointer dereference, address: 00000000
On Fri, 2020-04-24 at 12:16 +0200, Greg KH wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Fri, Apr 24, 2020 at 09:41:36AM +, > christian.gr...@microchip.com wrote: > > On Sun, 2020-03-29 at 21:39 +0800, kernel test robot wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > > know the content is safe > > > > > > Greetings, > > > > > > 0day kernel testing robot got the below dmesg and the first bad > > > commit is > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git > > > staging-next > > > > > > commit b276527539188f1f61c082ebef27803db93e536d > > > Author: Christian Gromm > > > AuthorDate: Tue Mar 10 14:02:40 2020 +0100 > > > Commit: Greg Kroah-Hartman > > > CommitDate: Tue Mar 24 13:42:44 2020 +0100 > > > > > > staging: most: move core files out of the staging area > > > > > > This patch moves the core module to the /drivers/most > > > directory > > > and makes all necessary changes in order to not break the > > > build. > > > > > > Signed-off-by: Christian Gromm > > > > > > Link: > > > https://lore.kernel.org/r/1583845362-26707-2-git-send-email-christian.gr...@microchip.com > > > Signed-off-by: Greg Kroah-Hartman > > > > > > > > > 22dd4acc80 Staging: speakup: Add identifier name to function > > > declaration arguments. > > > b276527539 staging: most: move core files out of the staging > > > area > > > e681bb287f staging: vt6656: Use DIV_ROUND_UP macro instead of > > > specific code > > > +---+-- > > > > > > --+++ > > > > | > > > > 22dd4acc80 > > > > > b276527539 | e681bb287f | > > > +---+-- > > > > > > --+++ > > > > boot_successes| > > > > 26 | 0 | 0 | > > > > boot_failures | > > > > 8 | 11 | 11 | > > > > WARNING:possible_circular_locking_dependency_detected | > > > > 8 ||| > > > > BUG:kernel_NULL_pointer_dereference,address | > > > > 0 | 11 | 11 | > > > > Oops:#[##]| > > > > 0 | 11 | 11 | > > > > EIP:__list_add_valid | > > > > 0 | 11 | 11 | > > > > Kernel_panic-not_syncing:Fatal_exception | > > > > 0 | 11 | 11 | > > > +---+-- > > > > > > --+++ > > > > > > If you fix the issue, kindly add following tag > > > Reported-by: kernel test robot > > > > > > [ 12.242090] no options. > > > [ 12.245364] FPGA DOWNLOAD ---> > > > [ 12.245723] FPGA image file name: xlinx_fpga_firmware.bit > > > [ 12.246548] GPIO INIT FAIL!! > > > [ 12.246995] most_sound: init() > > > [ 12.247349] BUG: kernel NULL pointer dereference, address: > > > > The init order of the modules is wrong in case the driver is > > being built in-tree. > > > > The init function of module most_sound is called before the > > core itself is being initialized. > > > > [5.179189] most_sound: init() > > [5.180205] mostcore: __init() > > > > Hence the list used in the core to store and track the > > registered components has not been initialized with > > INIT_LIST_HEAD(_list) by the time the sound module > > tries to register itself with the core. > > > > The Kconfig of most_sound, however, has a dependency to > > MOST. How can the build system be forced to initialize the > > core module first? > > Linker order is the thing here. > > You can mess with the init levels here, and use subsys_initcall() for > mostcore, will that fix it? I already gave it a try and it works. But is it ok to use the subsys_initcall() function when the driver is being built as a module? thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: b276527539 ("staging: most: move core files out of the staging .."): [ 12.247349] BUG: kernel NULL pointer dereference, address: 00000000
On Sun, 2020-03-29 at 21:39 +0800, kernel test robot wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > Greetings, > > 0day kernel testing robot got the below dmesg and the first bad > commit is > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git > staging-next > > commit b276527539188f1f61c082ebef27803db93e536d > Author: Christian Gromm > AuthorDate: Tue Mar 10 14:02:40 2020 +0100 > Commit: Greg Kroah-Hartman > CommitDate: Tue Mar 24 13:42:44 2020 +0100 > > staging: most: move core files out of the staging area > > This patch moves the core module to the /drivers/most directory > and makes all necessary changes in order to not break the build. > > Signed-off-by: Christian Gromm > Link: > https://lore.kernel.org/r/1583845362-26707-2-git-send-email-christian.gr...@microchip.com > Signed-off-by: Greg Kroah-Hartman > > 22dd4acc80 Staging: speakup: Add identifier name to function > declaration arguments. > b276527539 staging: most: move core files out of the staging area > e681bb287f staging: vt6656: Use DIV_ROUND_UP macro instead of > specific code > +---+-- > --+++ > > | 22dd4acc80 > > | b276527539 | e681bb287f | > +---+-- > --+++ > > boot_successes| > > 26 | 0 | 0 | > > boot_failures | > > 8 | 11 | 11 | > > WARNING:possible_circular_locking_dependency_detected | > > 8 ||| > > BUG:kernel_NULL_pointer_dereference,address | > > 0 | 11 | 11 | > > Oops:#[##]| > > 0 | 11 | 11 | > > EIP:__list_add_valid | > > 0 | 11 | 11 | > > Kernel_panic-not_syncing:Fatal_exception | > > 0 | 11 | 11 | > +---+-- > --+++ > > If you fix the issue, kindly add following tag > Reported-by: kernel test robot > > [ 12.242090] no options. > [ 12.245364] FPGA DOWNLOAD ---> > [ 12.245723] FPGA image file name: xlinx_fpga_firmware.bit > [ 12.246548] GPIO INIT FAIL!! > [ 12.246995] most_sound: init() > [ 12.247349] BUG: kernel NULL pointer dereference, address: The init order of the modules is wrong in case the driver is being built in-tree. The init function of module most_sound is called before the core itself is being initialized. [5.179189] most_sound: init() [5.180205] mostcore: __init() Hence the list used in the core to store and track the registered components has not been initialized with INIT_LIST_HEAD(_list) by the time the sound module tries to register itself with the core. The Kconfig of most_sound, however, has a dependency to MOST. How can the build system be forced to initialize the core module first? thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: b276527539 ("staging: most: move core files out of the staging .."): [ 12.247349] BUG: kernel NULL pointer dereference, address: 00000000
On Sun, 2020-03-29 at 21:39 +0800, kernel test robot wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > Greetings, > > 0day kernel testing robot got the below dmesg and the first bad > commit is > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git > staging-next > > commit b276527539188f1f61c082ebef27803db93e536d > Author: Christian Gromm > AuthorDate: Tue Mar 10 14:02:40 2020 +0100 > Commit: Greg Kroah-Hartman > CommitDate: Tue Mar 24 13:42:44 2020 +0100 > > staging: most: move core files out of the staging area > > This patch moves the core module to the /drivers/most directory > and makes all necessary changes in order to not break the build. > > Signed-off-by: Christian Gromm > Link: > https://lore.kernel.org/r/1583845362-26707-2-git-send-email-christian.gr...@microchip.com > Signed-off-by: Greg Kroah-Hartman > > 22dd4acc80 Staging: speakup: Add identifier name to function > declaration arguments. > b276527539 staging: most: move core files out of the staging area > e681bb287f staging: vt6656: Use DIV_ROUND_UP macro instead of > specific code > +---+-- > --+++ > > | 22dd4acc80 > > | b276527539 | e681bb287f | > +---+-- > --+++ > > boot_successes| > > 26 | 0 | 0 | > > boot_failures | > > 8 | 11 | 11 | > > WARNING:possible_circular_locking_dependency_detected | > > 8 ||| > > BUG:kernel_NULL_pointer_dereference,address | > > 0 | 11 | 11 | > > Oops:#[##]| > > 0 | 11 | 11 | > > EIP:__list_add_valid | > > 0 | 11 | 11 | > > Kernel_panic-not_syncing:Fatal_exception | > > 0 | 11 | 11 | > +---+-- > --+++ > > If you fix the issue, kindly add following tag > Reported-by: kernel test robot > > [ 12.242090] no options. > [ 12.245364] FPGA DOWNLOAD ---> > [ 12.245723] FPGA image file name: xlinx_fpga_firmware.bit > [ 12.246548] GPIO INIT FAIL!! > [ 12.246995] most_sound: init() > [ 12.247349] BUG: kernel NULL pointer dereference, address: > > [ 12.248032] #PF: supervisor read access in kernel mode > [ 12.248322] #PF: error_code(0x) - not-present page > [ 12.248322] *pdpt = *pde = f000ff53f000ff53 > [ 12.248322] Oops: [#1] PREEMPT SMP > [ 12.248322] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc7- > 00376-gb276527539188 #1 > [ 12.248322] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), > BIOS 1.12.0-1 04/01/2014 > [ 12.248322] EIP: __list_add_valid+0x29/0x77 > [ 12.248322] Code: c3 55 89 e5 56 53 83 ec 10 8b 59 04 39 d3 74 1a > 89 4c 24 0c 89 5c 24 08 89 54 24 04 c7 04 24 00 cc bd c2 e8 84 9e b4 > ff 0f 0b <8b> 33 39 ce 74 1a 89 5c 24 0c 89 74 24 08 89 4c 24 04 c7 > 04 24 7c > [ 12.248322] EAX: c2f45800 EBX: ECX: c3e8df50 EDX: > > [ 12.248322] ESI: EDI: ec4a7f68 EBP: ec4a7ee8 ESP: > ec4a7ed0 > [ 12.248322] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: > 00010246 > [ 12.248322] CR0: 80050033 CR2: CR3: 03256000 CR4: > 001406b0 > [ 12.248322] Call Trace: > [ 12.248322] ? vprintk_func+0x9d/0xa7 > [ 12.248322] most_register_component+0x33/0x53 This function does a NULL check on the passed argument struct most_component, berfore it calls list_add_tail(). So the dereferenced pointer must be the struct list_head comp_list of the core. > [ 12.248322] ? wilc_spi_driver_init+0x11/0x11 > [ 12.248322] audio_init+0x2c/0x76 > [ 12.248322] do_one_initcall+0xf0/0x284 > [ 12.248322] ? __might_sleep+0x70/0x77 > [ 12.262064] kernel_init_freeable+0x141/0x1a5 > [ 12.262064] ? rest_init+0x205/0x205 > [ 12.262064] kernel_init+0xb/0xea > [ 12.262064] ? schedule_tail_wrapper+0x9/0xc > [ 12.262064] ret_from_fork+0x2e/0x38 > [ 12.262064] Modules linked in: > [ 12.262064] CR2: > [ 12.262064] ---[ end trace 7c7a2cb6d11f9c5d ]--- > [ 12.262064] EIP: __list_add_valid+0x29/0x77 which is weird, as the list_head used here is not dynamically allocated and INIT_LIST_HEAD is definitely being called in the __init function most_init() of the core module before its first usage. I've never seen the code failing at this point, nor has this being reported by anyone yet. Need to investigate. thanks, Chris ___ devel mailing list
Re: [staging:staging-testing 278/280] drivers/most/core.c:1287 most_register_interface() error: we previously assumed 'iface' could be null (see line 1285)
On Tue, 2020-03-31 at 13:45 +0300, Dan Carpenter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> Hi Christian,
>
> First bad commit (maybe != root cause):
>
> tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> staging-testing
> head: 4a1a3e9bf5654e98bb48f5b074af17af96ded30d
> commit: b276527539188f1f61c082ebef27803db93e536d [278/280] staging:
> most: move core files out of the staging area
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot
> Reported-by: Dan Carpenter
>
> smatch warnings:
> drivers/most/core.c:1287 most_register_interface() error: we
> previously assumed 'iface' could be null (see line 1285)
>
> #
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?id=b276527539188f1f61c082ebef27803db93e536d
> git remote add staging
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> git remote update staging
> git checkout b276527539188f1f61c082ebef27803db93e536d
> vim +/iface +1287 drivers/most/core.c
>
> 4d5f022f3a664e drivers/staging/most/core.c Christian
> Gromm 2017-11-21 1279 int most_register_interface(struct
> most_interface *iface)
> 57562a72414ca3 drivers/staging/most/mostcore/core.c Christian
> Gromm 2015-07-24 1280 {
> 57562a72414ca3 drivers/staging/most/mostcore/core.c Christian
> Gromm 2015-07-24 1281unsigned int i;
> 57562a72414ca3 drivers/staging/most/mostcore/core.c Christian
> Gromm 2015-07-24 1282int id;
> fcb7fad82e23f6 drivers/staging/most/core.c Christian
> Gromm 2017-11-21 1283struct most_channel *c;
> 57562a72414ca3 drivers/staging/most/mostcore/core.c Christian
> Gromm 2015-07-24 1284
> 57562a72414ca3 drivers/staging/most/mostcore/core.c Christian
> Gromm 2015-07-24 @1285if (!iface || !iface->enqueue ||
> !iface->configure ||
>
>^^
>
> 57562a72414ca3 drivers/staging/most/mostcore/core.c Christian
> Gromm 2015-07-24 1286!iface->poison_channel ||
> (iface->num_channels > MAX_CHANNELS)) {
> 6a82c775812944 drivers/staging/most/core.c Christian
> Gromm 2020-01-23 @1287dev_err(iface->dev, "Bad
> interface or channel overflow\n");
>
>^^
>
> Hopefully, we can just remove the NULL check?
Yes we can. The modules that register such an interface with the
core already check that.
I'll send a patch that removes the NULL check of the "iface" pointer.
thanks,
Chris
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RESEND PATCH v5 0/3] staging: most: move core module out of staging
On Tue, 2020-03-24 at 13:44 +0100, Greg KH wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Tue, Mar 10, 2020 at 02:02:39PM +0100, Christian Gromm wrote: > > v2: > > Reported-by: Greg Kroah-Hartman > > - use -M option to create patches > > v3: > > Reported-by: Greg Kroah-Hartman > > - fix date range in comment section of core.c > > - move code to free up memory to release funtions > > - remove noisy log messages > > - use dev_* functions for logging > > v4: > > Reported-by: Greg Kroah-Hartman > > - change owner of struct device that is registered with > > kernel's > > device/driver model > > - fix linked list race condition > > - fix logging behaviour > > - fix possible NULL pointer dereference > > v5: > > rebased and adapted > > Sorry for the long delay on this, looks good, thanks for sticking > with > this. > > All now queued up. Nice! :) Thank you very much, Chris > > greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 01/10] staging: most: remove device from interface structure
On Fri, 2020-01-24 at 10:09 +0100, Greg KH wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > > Ok, I'll take the first 7 of these patches and see what the end > result > looks like after that, it will make reviewing the code easier... Do you have a rough idea when I can expect this to be reviewed? Should I resend one more time? thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 01/10] staging: most: remove device from interface structure
On Fri, 2020-01-24 at 10:09 +0100, Greg KH wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Fri, Jan 24, 2020 at 08:56:56AM +, > christian.gr...@microchip.com wrote: > > On Thu, 2020-01-23 at 19:18 +0100, Greg KH wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > > know the content is safe > > > > > > On Thu, Jan 23, 2020 at 04:38:17PM +0100, Christian Gromm wrote: > > > > This patch makes the adapter drivers use their own device > > > > structures > > > > when registering a most interface with the core module. > > > > With this the module that actually operates the physical device > > > > is > > > > the > > > > owner of the device. > > > > > > Ick, why? The interface should be part of sysfs, right, and now > > > it > > > isn't? > > > > It still is. What has changed is that the device that actually > > represents the attached hardware is used (see probe function of > > the USB adapter driver for instance). > > Ah. Ick. odd... > > > > Who handles the lifetime rules of these interfaces now? Why > > > remove this? > > > > The struct device that is allocated when attaching a MOST device is > > handling the lifetime and the struct most_interface is > > representing this device in the kernel. Hence, registered with > > sysfs. > > > > This ensures that the device is present in the kernel until its > > physical stature is being detached from the system. > > The core driver is just the man in the middle that registers the > > bus and itself as the driver and organizes the configfs, sysfs and > > communication paths to user space. > > > > > > > > Why isn't the interface dynamically created properly? That > > > should > > > solve > > > the lifetime rules here, right? > > > > The interface is dynamically allocated. This happens inside the > > USB, DIM2, I2C etc. drivers. The struct most_interface is part of > > the container struct there. > > Ok, I'll take the first 7 of these patches and see what the end > result > looks like after that, it will make reviewing the code easier... Did you find some time for the review yet? thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 01/10] staging: most: remove device from interface structure
On Thu, 2020-01-23 at 19:18 +0100, Greg KH wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Thu, Jan 23, 2020 at 04:38:17PM +0100, Christian Gromm wrote: > > This patch makes the adapter drivers use their own device > > structures > > when registering a most interface with the core module. > > With this the module that actually operates the physical device is > > the > > owner of the device. > > Ick, why? The interface should be part of sysfs, right, and now it > isn't? It still is. What has changed is that the device that actually represents the attached hardware is used (see probe function of the USB adapter driver for instance). > Who handles the lifetime rules of these interfaces now? Why > remove this? The struct device that is allocated when attaching a MOST device is handling the lifetime and the struct most_interface is representing this device in the kernel. Hence, registered with sysfs. This ensures that the device is present in the kernel until its physical stature is being detached from the system. The core driver is just the man in the middle that registers the bus and itself as the driver and organizes the configfs, sysfs and communication paths to user space. > > Why isn't the interface dynamically created properly? That should > solve > the lifetime rules here, right? The interface is dynamically allocated. This happens inside the USB, DIM2, I2C etc. drivers. The struct most_interface is part of the container struct there. thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC v3 4/9] staging: most: move interface dev to private section
On Wed, 2020-01-15 at 13:17 +0100, Greg KH wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Tue, Jan 14, 2020 at 04:57:53PM +0100, Christian Gromm wrote: > > This patch moves the struct device of the interface structure to > > its > > private section, because only the core should access it directly. > > For > > other entities an API is provided. > > This feels "wrong". > > Shouldn't the struct device in this structure be the thing that is > handling the reference counting and sysfs integration for this > structure? Yes, that's right. > To put it as a "pointer" in a private section of the > structure feels like it is now backwards. > > What is this helping with? Who was messing with the device structure > here that needed to not mess with it? Well, it's not that somebody was messing with it. It's just the fact that somebody could. > > It's good that you are now releasing the memory for the device > structure > properly, but this still feels really wrong. What is keeping the > lifetime of this structure now that the device is removed from it? It's the most_dev structure of the of USB module (or any other lower adapter driver), which embeds the most_interface sturcture that contained the dev struct (which I moved to the private section). The thing that might be confusing is that the place, where the parent structure with the device is being allocated, is not the same where the device actually gets registered with the kernel. These are two different kernel modules actually. thanks, Chris > > thanks, > > greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC v3 5/9] staging: most: usb: check for NULL device
On Wed, 2020-01-15 at 13:18 +0100, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Tue, Jan 14, 2020 at 04:57:54PM +0100, Christian Gromm wrote:
> > Check if the dci structer has been allocated before trying to
> > release it.
> >
> > Signed-off-by: Christian Gromm
> > ---
> > v3:
> > This patch has been added to the series.
> >
> > drivers/staging/most/usb/usb.c | 6 --
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/most/usb/usb.c
> > b/drivers/staging/most/usb/usb.c
> > index fe3384a..cae7553 100644
> > --- a/drivers/staging/most/usb/usb.c
> > +++ b/drivers/staging/most/usb/usb.c
> > @@ -1205,8 +1205,10 @@ static void hdm_disconnect(struct
> > usb_interface *interface)
> > del_timer_sync(>link_stat_timer);
> > cancel_work_sync(>poll_work_obj);
> >
> > - most_put_iface_dev(mdev->dci->dev.parent);
> > - device_unregister(>dci->dev);
> > + if (mdev->dci) {
> > + most_put_iface_dev(mdev->dci->dev.parent);
> > + device_unregister(>dci->dev);
> > + }
>
> How can this happen?
Depending on the Vendor/Product ID pair of the device that
probes the USB driver, this 'dci' structure is being allocated
and registered or not. And that is why this check is necessary.
>
> And why is it up to the child function here to unregister the device,
Child function? The device is being registered in the probe function
and unregistered in the disconnect function. What is wrong here?
thanks,
Chris
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC v2 7/9] staging: most: move core files out of the staging area
On Wed, 2019-12-18 at 16:02 +0100, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Wed, Dec 18, 2019 at 02:50:32PM +,
> christian.gr...@microchip.com wrote:
> > On Wed, 2019-12-18 at 15:08 +0100, Greg KH wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > >
> > > On Wed, Dec 18, 2019 at 02:02:43PM +,
> > > christian.gr...@microchip.com wrote:
> > > > On Tue, 2019-12-17 at 14:05 +0100, Greg KH wrote:
> > > > > EXTERNAL EMAIL: Do not click links or open attachments unless
> > > > > you
> > > > > know the content is safe
> > > > >
> > > > > On Fri, Dec 13, 2019 at 01:04:20PM +0100, Christian Gromm
> > > > > wrote:
> > > > > > This patch moves the core module to the /drivers/most
> > > > > > directory
> > > > > > and makes all necessary changes in order to not break the
> > > > > > build.
> > > > > >
> > > > > > Signed-off-by: Christian Gromm <
> > > > > > christian.gr...@microchip.com>
> > > > >
> > > > > I've applied the patches up to this one in the series, but I
> > > > > still
> > > > > have
> > > > > questions about the file you are trying to move here.
> > > > >
> > > > > It's not in this patch, but I'll just quote from the file
> > > > > drivers/staging/most/core.c directly:
> > > > >
> > > > > * Copyright (C) 2013-2015 Microchip Technology Germany II
> > > > > GmbH &
> > > > > Co.
> > > > > KG
> > > > >
> > > > > You've touched this file since 2015 :)
> > > > >
> > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > >
> > > > > No need for this, You have drivers here, no need to use any
> > > > > pr_*
> > > > > calls,
> > > > > as you always have a device structure.
> > > > > Along with that, almost all of your pr_info() calls are
> > > > > really
> > > > > errors/warnigns, so use dev_err() or dev_warn() instead
> > > > > please.
> > > > >
> > > > > The one:
> > > > > pr_info("registered new core component %s\n", comp->name);
> > > > >
> > > > > Should at best be a dev_info() line, but really, you don't
> > > > > need
> > > > > to be
> > > > > loud if all goes well, right?
> > > > >
> > > > > pr_info("deregistering component %s\n", comp->name);
> > > > >
> > > > > Should be dev_dbg().
> > > > >
> > > > > static void release_interface(struct device *dev)
> > > > > {
> > > > > pr_info("releasing interface dev %s...\n",
> > > > > dev_name(dev));
> > > > > }
> > > > >
> > > > > static void release_channel(struct device *dev)
> > > > > {
> > > > > pr_info("releasing channel dev %s...\n",
> > > > > dev_name(dev));
> > > > > }
> > > > >
> > > > > How did I miss this before?
> > > > >
> > > > > The driver core documentation used to have a line saying I
> > > > > was
> > > > > allowed
> > > > > to make fun of programmers who did this, but that had to be
> > > > > removed
> > > > > :(
> > > > >
> > > > > Anyway, this is totally wrong, first off, delete the
> > > > > debugging
> > > > > lines.
> > > > > Secondly how are you really releasing anything?
> > > >
> > > > Allocated memory is being freed inside the deregister*
> > > > functions,
> > > > once a device is detached from the system or the physical
> > > > adapter
> > > > driver has been removed. There a loop frees all channels and
> > > > interfaces
> > > > and the devices are unregistered with the kernel.
> > > >
> > > > I can move this to the release functions.
> > >
> > > It has to go there, as you have no idea if someone else has a
> > > reference
> > > to those structures. You have to abide by the fact that they are
> > > dynamic reference-counted structures, and that means you never
> > > "know"
> > > what the reference count is :)
> > >
> > > > > You have to free the
> > > > > memory here. You can not have an "empty" release function,
> > > > > the
> > > > > driver
> > > > > core requires you to actually do something here.
> > > > >
> > > > > Same for release_most_sub() and anywhere else I missed in my
> > > > > review.
> > > >
> > > > Here no memory has been allocated dynamically. What am I
> > > > supposed
> > > > to
> > > > free up?
> > >
> > > You have a structure that is reference counted, it had to be
> > > allocated
> > > dynamically, otherwise why is there a release function?
> >
> > Actually, no! The release function is there, because I have
> > a struct device embedded. And the kernel prints this
> > "scary complaint", when I try to register it with no release
> > function assigned. :)
>
> Stop and think _why_ someone (i.e. me) took the time and energy to
> write
> code to have the kernel print out that scary complaint. It wasn't
> just
> because I had nothing better to do...
>
> I wrote that code in order to tell people "hey, your code is buggy,
> fix
> it properly!" I didn't do that to tell people, "hey, provide an
> empty
> release function to quiet this foolish warning that I should never
> have
> added!"
>
> When the kernel complains about
Re: [PATCH RFC v2 7/9] staging: most: move core files out of the staging area
On Wed, 2019-12-18 at 15:08 +0100, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Wed, Dec 18, 2019 at 02:02:43PM +,
> christian.gr...@microchip.com wrote:
> > On Tue, 2019-12-17 at 14:05 +0100, Greg KH wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > >
> > > On Fri, Dec 13, 2019 at 01:04:20PM +0100, Christian Gromm wrote:
> > > > This patch moves the core module to the /drivers/most directory
> > > > and makes all necessary changes in order to not break the
> > > > build.
> > > >
> > > > Signed-off-by: Christian Gromm
> > >
> > > I've applied the patches up to this one in the series, but I
> > > still
> > > have
> > > questions about the file you are trying to move here.
> > >
> > > It's not in this patch, but I'll just quote from the file
> > > drivers/staging/most/core.c directly:
> > >
> > > * Copyright (C) 2013-2015 Microchip Technology Germany II GmbH &
> > > Co.
> > > KG
> > >
> > > You've touched this file since 2015 :)
> > >
> > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > >
> > > No need for this, You have drivers here, no need to use any pr_*
> > > calls,
> > > as you always have a device structure.
> > > Along with that, almost all of your pr_info() calls are really
> > > errors/warnigns, so use dev_err() or dev_warn() instead please.
> > >
> > > The one:
> > > pr_info("registered new core component %s\n", comp->name);
> > >
> > > Should at best be a dev_info() line, but really, you don't need
> > > to be
> > > loud if all goes well, right?
> > >
> > > pr_info("deregistering component %s\n", comp->name);
> > >
> > > Should be dev_dbg().
> > >
> > > static void release_interface(struct device *dev)
> > > {
> > > pr_info("releasing interface dev %s...\n",
> > > dev_name(dev));
> > > }
> > >
> > > static void release_channel(struct device *dev)
> > > {
> > > pr_info("releasing channel dev %s...\n", dev_name(dev));
> > > }
> > >
> > > How did I miss this before?
> > >
> > > The driver core documentation used to have a line saying I was
> > > allowed
> > > to make fun of programmers who did this, but that had to be
> > > removed
> > > :(
> > >
> > > Anyway, this is totally wrong, first off, delete the debugging
> > > lines.
> > > Secondly how are you really releasing anything?
> >
> > Allocated memory is being freed inside the deregister* functions,
> > once a device is detached from the system or the physical adapter
> > driver has been removed. There a loop frees all channels and
> > interfaces
> > and the devices are unregistered with the kernel.
> >
> > I can move this to the release functions.
>
> It has to go there, as you have no idea if someone else has a
> reference
> to those structures. You have to abide by the fact that they are
> dynamic reference-counted structures, and that means you never "know"
> what the reference count is :)
>
> > > You have to free the
> > > memory here. You can not have an "empty" release function, the
> > > driver
> > > core requires you to actually do something here.
> > >
> > > Same for release_most_sub() and anywhere else I missed in my
> > > review.
> >
> > Here no memory has been allocated dynamically. What am I supposed
> > to
> > free up?
>
> You have a structure that is reference counted, it had to be
> allocated
> dynamically, otherwise why is there a release function?
Actually, no! The release function is there, because I have
a struct device embedded. And the kernel prints this
"scary complaint", when I try to register it with no release
function assigned. :)
>
> > I've seen code in the kernel that does host empty release
> > functions.
>
> Where? I'll go yell at them :)
drivers/usb/gadget/udc/core.c
drivers/media/platform/vicodec/vicodec-core.c
maybe more...
thanks,
Chris
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC v2 7/9] staging: most: move core files out of the staging area
On Tue, 2019-12-17 at 14:05 +0100, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Fri, Dec 13, 2019 at 01:04:20PM +0100, Christian Gromm wrote:
> > This patch moves the core module to the /drivers/most directory
> > and makes all necessary changes in order to not break the build.
> >
> > Signed-off-by: Christian Gromm
>
> I've applied the patches up to this one in the series, but I still
> have
> questions about the file you are trying to move here.
>
> It's not in this patch, but I'll just quote from the file
> drivers/staging/most/core.c directly:
>
> * Copyright (C) 2013-2015 Microchip Technology Germany II GmbH & Co.
> KG
>
> You've touched this file since 2015 :)
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> No need for this, You have drivers here, no need to use any pr_*
> calls,
> as you always have a device structure.
> Along with that, almost all of your pr_info() calls are really
> errors/warnigns, so use dev_err() or dev_warn() instead please.
>
> The one:
> pr_info("registered new core component %s\n", comp->name);
>
> Should at best be a dev_info() line, but really, you don't need to be
> loud if all goes well, right?
>
> pr_info("deregistering component %s\n", comp->name);
>
> Should be dev_dbg().
>
> static void release_interface(struct device *dev)
> {
> pr_info("releasing interface dev %s...\n", dev_name(dev));
> }
>
> static void release_channel(struct device *dev)
> {
> pr_info("releasing channel dev %s...\n", dev_name(dev));
> }
>
> How did I miss this before?
>
> The driver core documentation used to have a line saying I was
> allowed
> to make fun of programmers who did this, but that had to be removed
> :(
>
> Anyway, this is totally wrong, first off, delete the debugging lines.
> Secondly how are you really releasing anything?
Allocated memory is being freed inside the deregister* functions,
once a device is detached from the system or the physical adapter
driver has been removed. There a loop frees all channels and interfaces
and the devices are unregistered with the kernel.
I can move this to the release functions.
> You have to free the
> memory here. You can not have an "empty" release function, the
> driver
> core requires you to actually do something here.
>
> Same for release_most_sub() and anywhere else I missed in my review.
Here no memory has been allocated dynamically. What am I supposed to
free up?
I've seen code in the kernel that does host empty release functions.
That's probably why I didn't recognized this as a red flag or sensed
any bad feelings.
thanks,
Chris
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC 2/6] staging: most: rename core.h to most.h
On Fr, 2019-12-06 at 15:35 +0100, Greg KH wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Mon, Nov 25, 2019 at 04:51:32PM +0100, Christian Gromm wrote:
> >
> > This patch renames the core header file core.h to most.h. The
> > intention
> > behind this is to have a meaningful name once this file is moved to
> > the
> > /include/linux directory.
> Does everything in this .h file have to be exposed to the whole
> kernel?
> Are there any things in here that are local only to the most "core"
> code?
>
> If this whole thing is public, you might want to clean up your naming
> of
> some structures:
Yes, those structs are needed by modules registering with the core.
Do you want me to reroll v2 of this set with these changes or to wait
for more comments to come?
thanks,
Chris
>
> >
> > +enum mbo_status_flags {
> enum most_buffer_status_flags?
>
>
> >
> > +struct mbo {
> struct most_buffer_object?
>
> >
> > +struct core_component {
> struct most_core_component?
>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: most: configfs: PAGE_SIZE char arrays?
On Di, 2019-11-26 at 11:30 -0800, Joe Perches wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> drivers/staging/most/configfs.c:struct mdev_link {
> drivers/staging/most/configfs.c-struct config_item item;
> drivers/staging/most/configfs.c-struct list_head list;
> drivers/staging/most/configfs.c-bool create_link;
> drivers/staging/most/configfs.c-bool destroy_link;
> drivers/staging/most/configfs.c-u16 num_buffers;
> drivers/staging/most/configfs.c-u16 buffer_size;
> drivers/staging/most/configfs.c-u16 subbuffer_size;
> drivers/staging/most/configfs.c-u16 packets_per_xact;
> drivers/staging/most/configfs.c-u16 dbr_size;
> drivers/staging/most/configfs.c-char datatype[PAGE_SIZE];
> drivers/staging/most/configfs.c-char direction[PAGE_SIZE];
> drivers/staging/most/configfs.c-char name[PAGE_SIZE];
> drivers/staging/most/configfs.c-char device[PAGE_SIZE];
> drivers/staging/most/configfs.c-char channel[PAGE_SIZE];
> drivers/staging/most/configfs.c-char comp[PAGE_SIZE];
> drivers/staging/most/configfs.c-char comp_params[PAGE_SIZE];
> drivers/staging/most/configfs.c-};
>
> Why are all the char arrays size PAGE_SIZE ?
> That seems completely unnecessary.
Right. Probably a bad habit when handling userspace.
thanks,
Chris
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: most: usb: Remove variable frame_size
On Do, 2019-05-23 at 18:51 +0200, Greg KH wrote:
> External E-Mail
>
>
> On Thu, May 23, 2019 at 06:53:34PM +0530, Nishka Dasgupta wrote:
> >
> > Remove variable frame_size as its multiple usages are all
> > independent of
> > each other and so can be returned separately.
> > Issue found with Coccinelle.
> >
> > Signed-off-by: Nishka Dasgupta
> > ---
> > drivers/staging/most/usb/usb.c | 16 ++--
> > 1 file changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/staging/most/usb/usb.c
> > b/drivers/staging/most/usb/usb.c
> > index 360cb5b7a10b..751e82cf66c5 100644
> > --- a/drivers/staging/most/usb/usb.c
> > +++ b/drivers/staging/most/usb/usb.c
> > @@ -186,32 +186,28 @@ static inline int start_sync_ep(struct
> > usb_device *usb_dev, u16 ep)
> > */
> > static unsigned int get_stream_frame_size(struct
> > most_channel_config *cfg)
> > {
> > - unsigned int frame_size = 0;
> > unsigned int sub_size = cfg->subbuffer_size;
> >
> > if (!sub_size) {
> > pr_warn("Misconfig: Subbuffer size zero.\n");
> > - return frame_size;
> > + return 0;
> > }
> > switch (cfg->data_type) {
> > case MOST_CH_ISOC:
> > - frame_size = AV_PACKETS_PER_XACT * sub_size;
> > - break;
> > + return AV_PACKETS_PER_XACT * sub_size;
> > case MOST_CH_SYNC:
> > if (cfg->packets_per_xact == 0) {
> > pr_warn("Misconfig: Packets per XACT
> > zero\n");
> > - frame_size = 0;
> > + return 0;
> > } else if (cfg->packets_per_xact == 0xFF) {
> > - frame_size = (USB_MTU / sub_size) *
> > sub_size;
> > + return (USB_MTU / sub_size) * sub_size;
> > } else {
> > - frame_size = cfg->packets_per_xact *
> > sub_size;
> > + return cfg->packets_per_xact * sub_size;
> > }
> > - break;
> > default:
> > pr_warn("Query frame size of non-streaming
> > channel\n");
> > - break;
> > + return 0;
> > }
> > - return frame_size;
> > }
> Now it just feels like you are doing "busy work" :(
>
> frame_size makes sense here, right? Why change this code?
>
> Remember, code is written for developers first, the compiler second.
> Reading this with frame_size makes it much more obvious what this
> code
> does when you read it again in 5-10 years. Why change this, you have
> not made it faster, or smaller at all.
>
> So no, I would not accept this, sorry.
I haven't seen this on time. So please ignore my acknowledge I've sent.
Good to know that you favor readability over compactness.
Sorry for the noise,
Chris
>
> We have so many _real_ things to do in the drivers/staging/ directory
> if
> you are looking for stuff to clean up. Don't try to micro-optimize
> things that do not matter at the expense of understanding.
>
> thanks,
>
> greg k-h
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: most: usb: Remove variable frame_size
On Do, 2019-05-23 at 18:53 +0530, Nishka Dasgupta wrote:
> External E-Mail
>
>
> Remove variable frame_size as its multiple usages are all independent
> of
> each other and so can be returned separately.
> Issue found with Coccinelle.
>
> Signed-off-by: Nishka Dasgupta
Acked-by: Christian Gromm
> ---
> drivers/staging/most/usb/usb.c | 16 ++--
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/most/usb/usb.c
> b/drivers/staging/most/usb/usb.c
> index 360cb5b7a10b..751e82cf66c5 100644
> --- a/drivers/staging/most/usb/usb.c
> +++ b/drivers/staging/most/usb/usb.c
> @@ -186,32 +186,28 @@ static inline int start_sync_ep(struct
> usb_device *usb_dev, u16 ep)
> */
> static unsigned int get_stream_frame_size(struct most_channel_config
> *cfg)
> {
> - unsigned int frame_size = 0;
> unsigned int sub_size = cfg->subbuffer_size;
>
> if (!sub_size) {
> pr_warn("Misconfig: Subbuffer size zero.\n");
> - return frame_size;
> + return 0;
> }
> switch (cfg->data_type) {
> case MOST_CH_ISOC:
> - frame_size = AV_PACKETS_PER_XACT * sub_size;
> - break;
> + return AV_PACKETS_PER_XACT * sub_size;
> case MOST_CH_SYNC:
> if (cfg->packets_per_xact == 0) {
> pr_warn("Misconfig: Packets per XACT
> zero\n");
> - frame_size = 0;
> + return 0;
> } else if (cfg->packets_per_xact == 0xFF) {
> - frame_size = (USB_MTU / sub_size) *
> sub_size;
> + return (USB_MTU / sub_size) * sub_size;
> } else {
> - frame_size = cfg->packets_per_xact *
> sub_size;
> + return cfg->packets_per_xact * sub_size;
> }
> - break;
> default:
> pr_warn("Query frame size of non-streaming
> channel\n");
> - break;
> + return 0;
> }
> - return frame_size;
> }
>
> /**
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: most: cdev: fix chrdev_region leak in mod_exit
On Mi, 2019-04-24 at 21:23 +0200, Eugeniu Rosca wrote:
> External E-Mail
>
>
> From: Suresh Udipi
>
> It looks like v4.18-rc1 commit [0] which upstreams mld-1.8.0
> commit [1] missed to fix the memory leak in mod_exit function.
>
> Do it now.
>
> [0] aba258b7310167 ("staging: most: cdev: fix chrdev_region leak")
> [1] https://github.com/microchip-ais/linux/commit/a2d8f7ae7ea381
> ("staging: most: cdev: fix leak for chrdev_region")
>
> Signed-off-by: Suresh Udipi
> Signed-off-by: Eugeniu Rosca
Acked-by: Christian Gromm
> ---
> drivers/staging/most/cdev/cdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/most/cdev/cdev.c
> b/drivers/staging/most/cdev/cdev.c
> index f2b347cda8b7..d5f236889021 100644
> --- a/drivers/staging/most/cdev/cdev.c
> +++ b/drivers/staging/most/cdev/cdev.c
> @@ -549,7 +549,7 @@ static void __exit mod_exit(void)
> destroy_cdev(c);
> destroy_channel(c);
> }
> - unregister_chrdev_region(comp.devno, 1);
> + unregister_chrdev_region(comp.devno, CHRDEV_REGION_SIZE);
> ida_destroy(_id);
> class_destroy(comp.class);
> }
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: most: sound: pass correct device when creating a sound card
On Di, 2019-04-30 at 11:20 +0200, Greg KH wrote: > External E-Mail > > > On Tue, Apr 30, 2019 at 11:00:22AM +0200, Christian Gromm wrote: > > > > This patch fixes the usage of the wrong struct device when calling > > function snd_card_new. > > > > Reported-by: Eugeniu Rosca > > Signed-off-by: Christian Gromm > > --- > > drivers/staging/most/sound/sound.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > Does this fix a specific commit? If so, should there be a "Fixes: " > tag > in the s-o-b: area? Does this need to go to the stable trees? Yes, yes and yes. I'll be sending a v2 shortly. I wasn't aware that I need to refer to a certain commit when fixing things up. How can bugfix patches not fix a specific commit anyway? The bugs must have gotten in somehow, right? thanks, Chris > > thanks, > > greg k-h > ___ > devel mailing list > de...@linuxdriverproject.org > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-de > vel > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 16/28] staging: most: sound: call snd_card_new with struct device
On Mi, 2019-04-24 at 20:50 +0200, Eugeniu Rosca wrote:
> External E-Mail
>
>
> Hi Christian,
>
> On Tue, 08 May 2018 02:46:44 -0700, Christian Gromm wrote:
> >
> > This patch is needed as function snd_card_new needs a valid
> > parent device. Passing a NULL pointer leads to kernel Ooops.
> >
> > Signed-off-by: Christian Gromm
> > ---
> > drivers/staging/most/core.h| 1 +
> > drivers/staging/most/sound/sound.c | 2 +-
> > drivers/staging/most/usb/usb.c | 1 +
> > 3 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/most/core.h
> > b/drivers/staging/most/core.h
> > index 7a3c70bead19..64cc02f161e7 100644
> > --- a/drivers/staging/most/core.h
> > +++ b/drivers/staging/most/core.h
> > @@ -230,6 +230,7 @@ struct mbo {
> > */
> > struct most_interface {
> > struct device dev;
> > + struct device *driver_dev;
> > struct module *mod;
> > enum most_interface_type interface;
> > const char *description;
> > diff --git a/drivers/staging/most/sound/sound.c
> > b/drivers/staging/most/sound/sound.c
> > index 18f722410a63..04c18323c2ea 100644
> > --- a/drivers/staging/most/sound/sound.c
> > +++ b/drivers/staging/most/sound/sound.c
> > @@ -590,7 +590,7 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> > if (ret < 0)
> > return ret;
> >
> > - ret = snd_card_new(NULL, -1, card_name, THIS_MODULE,
> > + ret = snd_card_new(>dev, -1, card_name,
> > THIS_MODULE,
> > sizeof(*channel), );
> > if (ret < 0)
> > return ret;
> > diff --git a/drivers/staging/most/usb/usb.c
> > b/drivers/staging/most/usb/usb.c
> > index 5ed1dccc0839..f18726049528 100644
> > --- a/drivers/staging/most/usb/usb.c
> > +++ b/drivers/staging/most/usb/usb.c
> > @@ -1043,6 +1043,7 @@ hdm_probe(struct usb_interface *interface,
> > const struct usb_device_id *id)
> > mdev->link_stat_timer.expires = jiffies + (2 * HZ);
> >
> > mdev->iface.mod = hdm_usb_fops.owner;
> > + mdev->iface.driver_dev = >dev;
> > mdev->iface.interface = ITYPE_USB;
> > mdev->iface.configure = hdm_configure_channel;
> > mdev->iface.request_netinfo = hdm_request_netinfo;
> Just for your information, when mapping commits from vanilla to those
> from https://github.com/microchip-ais/linux/commits/mld-1.8.0, we've
> stumbled upon some subtle but striking difference between mld-1.8.0
> commit [0] and v4.18-rc1 commit [1]. The latter looks like an
> upstreamed
> version of the former. However, while commit [0] creates a new 'dev'
> member in 'struct most_interface' and uses it consistently, commit
> [1]
> creates 'driver_dev' and uses it intermixed with 'dev'.
This looks sort of suspicious. I'll check it.
Thanks for letting me know.
Regards,
Chris
>
> Since we don't use aim-sound, we just signal this feedback to you
> as FWIW without sending a patch (which we can't test).
>
> [0] https://github.com/microchip-ais/linux/commit/2fef0f89f04703
> ("staging: most: add struct device to most interface")
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.gi
> t/commit/?id=69c90cf1b2faf5
> ("staging: most: sound: call snd_card_new with struct device")
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 12/12] staging: most: Documentation: update driver documentation
On Mo, 2019-04-15 at 23:21 +0200, Eugeniu Rosca wrote: > External E-Mail > > > Hello Christian, hello Andrey, > cc: Vladimir Barinov > cc: linux-renesas-soc > > Our team currently has the requirement of adding MOST support to > the v4.14-based R-Car3 kernel [1], matching the level of features > and interfaces of mld-1.8.0 [2] release. > > Based on a quick check [3], the mld-1.8.0 release contains 221 non- > merge > non-mainline commits. It seems like at least some (most?) of them > have > been reworked during upstreaming and are now part of vanilla, thanks > to > your efforts. > > Since you've actively participated in pushing the out-of-tree drivers > to mainline, could you please share your gut feeling whether the > current mainline state of the MOST subsystem matches the mld-1.8.0 > release in terms of features and interfaces? > No, it doesn't. The version upstream does not have all the bells and whistles the mld-1.8.0 version has, yet. Focus of the latest mainline commits was on the driver model and the switch to configfs. > At first glance, such mld-1.8.0 functionalities like "flexible PCM > rate support" [4], as well as a number of dim2 sysfs entries [5-8] > appear to be missing in v5.1-rc5. Could you please feedback if these > have been deliberately dropped or didn't make their way for another > reason? What would be your recommendation between going the mld-1.8.0 > and going the v5.1-rc5 way for MOST? > The functionalities you're referring to have _not_ intentionally been dropped. They will find their way into mainline. If there are urgent feature requests, we could prioritize them. Bottom line is, the upstream version is the one you should be using, as it is going to replace the mld-1.x branch. This is exactly what we will soon be doing for AGL by the way. thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 13/14] staging: most: configfs: rename config attributes
On Di, 2019-04-02 at 11:01 +0200, Greg KH wrote: > External E-Mail > > > On Tue, Apr 02, 2019 at 08:17:02AM +, Christian.Gromm@microchip.c > om wrote: > > > > On Di, 2019-04-02 at 09:25 +0200, Greg KH wrote: > > > > > > External E-Mail > > > > > > > > > On Mon, Apr 01, 2019 at 02:05:38PM +0200, Christian Gromm wrote: > > > > > > > > > > > > This patch introduces attribute names that are more self > > > > explaining. > > > > > > > > Signed-off-by: Christian Gromm > > > > --- > > > > v2: > > > > - follow-up adaptions due to changes introduced w/ > > > > [PATCH v2 > > > > 01/14] > > > > v3: > > > > > > > > drivers/staging/most/configfs.c | 97 +-- > > > > > > > > -- > > > > 1 file changed, 49 insertions(+), 48 deletions(-) > > > Why isn't this just part of patch 1/14 here? No need to create a > > > problem and then have to fix it up later in the same series :) > > > > > This is not part of patch 1/14, because it does a different thing. > > I just wanted to point out that this patch has small changes when > > "diffed" against v1. These changes have been introduced when I > > rebased the patch set with the changes in 1/14 recommended by Dan > > Carpenter. In principle, it does the very same as before. Nothing > > has been added. > My point is, why not add the "more self explaining" attribute names > the > first time around when you add the configfs.c file? Why add it and > then > later on change it? What did that benifit anyone? > Got your point. I'll fix it up. But then the patch series would be shorter as this patch is going to be merged with the first one. What does this mean to the reroll count? Can I do a v4 that has less patches than its predecessor? thanks, Chris > thanks, > > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 07/14] staging: most: core: use device description as name
On Di, 2019-04-02 at 11:00 +0200, Greg KH wrote: > External E-Mail > > > On Tue, Apr 02, 2019 at 08:10:25AM +, Christian.Gromm@microchip.c > om wrote: > > > > On Di, 2019-04-02 at 09:24 +0200, Greg KH wrote: > > > > > > External E-Mail > > > > > > > > > On Mon, Apr 01, 2019 at 02:05:32PM +0200, Christian Gromm wrote: > > > > > > > > > > > > This patch uses the device description to clearly identity a > > > > device > > > > attached to the bus. It is needed as the currently useed mdevX > > > > notation is not sufficiant in case more than one network > > > > interface controller is being used at the same time. > > > > > > > > Signed-off-by: Christian Gromm > > > > --- > > > > v2: > > > > v3: > > > > > > > > drivers/staging/most/core.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/staging/most/core.c > > > > b/drivers/staging/most/core.c > > > > index a5df53e..e0a6806 100644 > > > > --- a/drivers/staging/most/core.c > > > > +++ b/drivers/staging/most/core.c > > > > @@ -1467,7 +1467,7 @@ int most_register_interface(struct > > > > most_interface *iface) > > > > > > > > INIT_LIST_HEAD(>p->channel_list); > > > > iface->p->dev_id = id; > > > > - snprintf(iface->p->name, STRING_SIZE, "mdev%d", id); > > > > + strcpy(iface->p->name, iface->description); > > > > iface->dev.init_name = iface->p->name; > > > > iface->dev.bus = > > > > iface->dev.parent = > > > Is this a stand-alone bugfix that can be taken now and should > > > also be > > > added to the stable kernel trees? > > > > > Stable trees? The driver is not present there yet. > This driver has been in the kernel for many years, and this specific > file has been there since 4.16. We have long term stable releases > for > 4.19 going at the moment. > > If you don't care about the stable releases, that's fine, just asking > :) > Actually I do care (you know that). Just not sure if I understood what you was trying to tell me. To me "stable" means "not staging area". That's why I was confused. So yes please, add it. thanks, Chris > thanks, > > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 13/14] staging: most: configfs: rename config attributes
On Di, 2019-04-02 at 09:25 +0200, Greg KH wrote: > External E-Mail > > > On Mon, Apr 01, 2019 at 02:05:38PM +0200, Christian Gromm wrote: > > > > This patch introduces attribute names that are more self > > explaining. > > > > Signed-off-by: Christian Gromm > > --- > > v2: > > - follow-up adaptions due to changes introduced w/ [PATCH v2 > > 01/14] > > v3: > > > > drivers/staging/most/configfs.c | 97 +-- > > -- > > 1 file changed, 49 insertions(+), 48 deletions(-) > Why isn't this just part of patch 1/14 here? No need to create a > problem and then have to fix it up later in the same series :) > This is not part of patch 1/14, because it does a different thing. I just wanted to point out that this patch has small changes when "diffed" against v1. These changes have been introduced when I rebased the patch set with the changes in 1/14 recommended by Dan Carpenter. In principle, it does the very same as before. Nothing has been added. regards, Chris > thanks, > > greg k-h > ___ > devel mailing list > de...@linuxdriverproject.org > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-de > vel > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 07/14] staging: most: core: use device description as name
On Di, 2019-04-02 at 09:24 +0200, Greg KH wrote: > External E-Mail > > > On Mon, Apr 01, 2019 at 02:05:32PM +0200, Christian Gromm wrote: > > > > This patch uses the device description to clearly identity a device > > attached to the bus. It is needed as the currently useed mdevX > > notation is not sufficiant in case more than one network > > interface controller is being used at the same time. > > > > Signed-off-by: Christian Gromm > > --- > > v2: > > v3: > > > > drivers/staging/most/core.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/most/core.c > > b/drivers/staging/most/core.c > > index a5df53e..e0a6806 100644 > > --- a/drivers/staging/most/core.c > > +++ b/drivers/staging/most/core.c > > @@ -1467,7 +1467,7 @@ int most_register_interface(struct > > most_interface *iface) > > > > INIT_LIST_HEAD(>p->channel_list); > > iface->p->dev_id = id; > > - snprintf(iface->p->name, STRING_SIZE, "mdev%d", id); > > + strcpy(iface->p->name, iface->description); > > iface->dev.init_name = iface->p->name; > > iface->dev.bus = > > iface->dev.parent = > Is this a stand-alone bugfix that can be taken now and should also be > added to the stable kernel trees? > Stable trees? The driver is not present there yet. > thanks, > > greg k-h > ___ > devel mailing list > de...@linuxdriverproject.org > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-de > vel > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 05/14] staging: most: enable configfs support
On Di, 2019-04-02 at 09:20 +0200, Greg KH wrote: > External E-Mail > > > On Mon, Apr 01, 2019 at 02:05:30PM +0200, Christian Gromm wrote: > > > > This patch enables the configfs functionality of the driver by > > registering the configfs subsystems and compiling the configfs > > part of the sources. > > > > Signed-off-by: Christian Gromm > > --- > > v2: > > - remove call to configfs_exit function > > v3: > > > > drivers/staging/most/Makefile | 1 + > > drivers/staging/most/cdev/cdev.c | 6 ++ > > drivers/staging/most/core.c| 2 +- > > drivers/staging/most/sound/sound.c | 11 ++- > > 4 files changed, 18 insertions(+), 2 deletions(-) > You add the file in patch 1, but do not add it to the build until > patch > 5? Does that really work? > Yes it does, because user space can still use sysfs to configure the driver until patch 9/14 is being applied (which disables this function). I've done things this way to provide you with a clearly laid out patch queue. > And what about Kconfig dependancies, don't you need to add a > dependancy > on configfs to the most core now? > This dependency should be added, right. > thanks, > > greg k-h > ___ > devel mailing list > de...@linuxdriverproject.org > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-de > vel > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 04/14] staging: most: sound: introduce new sound adapter management
On Fr, 2019-03-29 at 16:50 +0300, Dan Carpenter wrote: > External E-Mail > > > Thanks, I feel like I understand better now. > > Sorry, I don't want to be a jerk and I'm not going to complain again > about this patchset if you resend it with the other stuff I mention > fixed. > > But to me it feels like the API could be improved slightly if you > first created the adapter with a name and then linked to > it. Creating > the adapter as a side effect of linking the first audio component > feels > like it seems helpful but it's going to be awkward. Hmm, I see. But I'm not sure if I can call snd_pcm_new() on an already registered sound adapter. > > I'm also concerned about the locking around the adpt_list. Is this > something ordinary users can trigger? Is it crash if I set my fuzz > tester to start randomly connecting and disconnecting like crazy? > Well, strictly speaking yes. Because it is user space. But normally, the driver is meant for engineered systems like a car (see Automotive Grade Linux). Here you will have a systemd unit that sets things up at system startup time. I don't expect having to deal with multiple user space applications that race about the sound adapters. But I will keep that in mind. thanks, Chris > regards, > dan carpenter > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 04/14] staging: most: sound: introduce new sound adapter management
On Fr, 2019-03-29 at 13:46 +0300, Dan Carpenter wrote:
> External E-Mail
>
>
> On Thu, Mar 28, 2019 at 02:17:32PM +0100, Christian Gromm wrote:
> >
> > +static int audio_create_sound_card(void)
> > +{
> > + int ret;
> > + struct sound_adapter *adpt;
> > +
> > + list_for_each_entry(adpt, _list, list) {
> > + if (!adpt->registered)
> > + goto adpt_alloc;
> > + }
> > + return -ENODEV;
> > +adpt_alloc:
> > + ret = snd_card_register(adpt->card);
> > + if (ret < 0) {
> > + release_adapter(adpt);
> > + return ret;
> > + }
> > + adpt->registered = true;
> > + return ret;
> ^^
>
> return 0;
>
> >
> > +}
> This function is just strange to me... We add a bunch of adapters to
> "adpt_list" in audio_probe_channel(). Each adapter has it's own
> adpt->card. But here we just take the first unregistered adapter and
> register it as our card.
>
Unfortunately, I am not an ASCII artist, otherwise I would have put
some flowcharts in here to make things clear.
But in principle, it is like this:
A channel of a device which is intended to carry audio data,
will be represented as a PCM device on a sound card. So, if
I want my sound card
--> mkdir /sys/kernel/config/most_sound/mycard
to have a capture device and a playback device, I would
link two channels (one rx and on tx) and hook them on the card by
creating a directory in the card's directory.
--> mkdir /sys/kernel/config/most_sound/mycard/pcm_capture
--> mkdir
/sys/kernel/config/most_sound/mycard/pcm_playback
Then I would go ahead and configure the attributes inside and
finally activate the links:
--> echo 1 > /sys/kernel/config/most_sound/mycard/pcm_playback/create_link
and
--> echo 1 > /sys/kernel/config/most_sound/mycard/pcm_capture/create_link
The first time a link is being activated, the probe function gets
called and if no adapter has yet been created, one is created now
and the PCM device is added. By the time the second call happens,
we already have an adapter that is _not_ registered with ALSA.
Hence, we skip allocating another adapter and use the one we got
to add the next PCM device.
Once I have all PCM devices set up, I would do
--> echo 1 > /sys/kernel/config/most_sound/mycard/create_card
And now, the "strange" audio_create_sound_card function is
called to register the adapter with ALSA. If everything is fine,
the adpt->registered flag is set and from now on I would be able
to create a second sound card on the system. An only now a new
adapter would be allocated in the probe function, because we
can't find one in our list that is waiting to get registered
with ALSA.
Unless I do the create_card thing, all channels would be added
to the same adapter and I would end up with a sound card that
has "a bunch" of PCM devices.
Hope I kind of could get the picture across.
thanks,
Chris
> regards,
> dan carpenter
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-de
> vel
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 04/14] staging: most: sound: introduce new sound adapter management
On Do, 2019-03-28 at 16:56 +0300, Dan Carpenter wrote:
> External E-Mail
>
>
> On Thu, Mar 28, 2019 at 02:17:32PM +0100, Christian Gromm wrote:
> >
> > +static int audio_create_sound_card(void)
> > +{
> > + int ret;
> > + struct sound_adapter *adpt;
> > +
> > + list_for_each_entry(adpt, _list, list) {
> > + if (!adpt->registered)
> > + goto adpt_alloc;
> > + }
> > + return -ENODEV;
> > +adpt_alloc:
> > + ret = snd_card_register(adpt->card);
> > + if (ret < 0) {
> > + release_adapter(adpt);
> This doesn't feel right. We didn't acquire "adpt" in this function
> so
> why are we releasing it here. Do we release it somewhere else as
> well?
>
We release the adapter, because it is useless if we have not been
able to register it with ALSA.
And yes, it is also being removed, when a channel gets disconnected.
> It's still on the list...
It is being removed from the list inside the function
release_adapter.
thanks,
Chris
>
> >
> > + return ret;
> > + }
> > + adpt->registered = true;
> > + return ret;
> > +}
> regards,
> dan carpenter
>
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 01/14] staging: most: add new file configfs.c
On Do, 2019-03-28 at 16:50 +0300, Dan Carpenter wrote:
> External E-Mail
>
>
> On Thu, Mar 28, 2019 at 02:17:29PM +0100, Christian Gromm wrote:
> >
> > +static ssize_t mdev_link_direction_store(struct config_item *item,
> > + const char *page, size_t
> > count)
> > +{
> > + struct mdev_link *mdev_link = to_mdev_link(item);
> > +
> > + if (sysfs_streq(page, "dir_rx") && sysfs_streq(page, "rx")
> > &&
> > + sysfs_streq(page, "dir_tx") && sysfs_streq(page,
> > "tx"))
> These tests are reversed. It will never return -EINVAL because one
> string can't be four things.
OMG, that was braindead. Sorry for the inconvenience. I'll fix that up.
thanks,
Chris
>
> if (!sysfs_streq(page, "dir_rx") && !sysfs_streq(page, "rx") &&
> !sysfs_streq(page, "dir_tx") && !sysfs_streq(page, "tx"))
> return -EINVAL;
>
> The sysfs_streq() return true if the strings are equal. The strcmp()
> functions less intuitive and they should be used like this:
>
> if (strcmp(foo, bar) < 0) { // <-- foo < bar
> if (strcmp(foo, bar) != 0) { // <-- foo != bar
> if (strcmp(foo, bar) == 0) { // <-- foo == bar
>
> The other streq() tests have the same issue.
>
> regards,
> dan carpenter
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 10/14] staging: most: allow speculative configuration
On Do, 2019-03-28 at 17:12 +0300, Dan Carpenter wrote:
> External E-Mail
>
>
> On Thu, Mar 28, 2019 at 02:17:38PM +0100, Christian Gromm wrote:
> >
> > This patch makes the driver accept a link confiiguration eventhough
> > no
> > device is attached to the bus. Instead the configuration is being
> > applied
> > as soon as a device is being registered with the core.
> >
> > Signed-off-by: Christian Gromm
> > ---
> > v2:
> > - follow-up adaptions due to changes introduced w/ [PATCH v2
> > 01/14]
> >
> > drivers/staging/most/configfs.c| 60
> > --
> > drivers/staging/most/core.c| 16 +++---
> > drivers/staging/most/core.h| 1 +
> > drivers/staging/most/sound/sound.c | 6 ++--
> > 4 files changed, 53 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/staging/most/configfs.c
> > b/drivers/staging/most/configfs.c
> > index 6968b299..3a7c54d 100644
> > --- a/drivers/staging/most/configfs.c
> > +++ b/drivers/staging/most/configfs.c
> > @@ -14,6 +14,7 @@
> >
> > struct mdev_link {
> > struct config_item item;
> > + struct list_head list;
> > bool create;
> > u16 num_buffers;
> > u16 buffer_size;
> > @@ -29,6 +30,8 @@ struct mdev_link {
> > char param[PAGE_SIZE];
> > };
> >
> > +struct list_head mdev_link_list;
> > +
> > static int set_cfg_buffer_size(struct mdev_link *link)
> > {
> > if (!link->buffer_size)
> > @@ -105,33 +108,41 @@ static ssize_t mdev_link_create_show(struct
> > config_item *item, char *page)
> > return snprintf(page, PAGE_SIZE, "%d\n",
> > to_mdev_link(item)->create);
> > }
> >
> > +static int set_config_and_add_link(struct mdev_link *mdev_link)
> > +{
> > + int i;
> > + int ret;
> > +
> > + for (i = 0; i < ARRAY_SIZE(set_config_val); i++) {
> > + ret = set_config_val[i](mdev_link);
> > + if (ret == -ENODATA) {
> I've read the commit description but I don't really understand the
> error codes. Here only -ENODATA is an error. But later -ENODEV
> is success. Why not "if (ret) {" here?
>
The most_set_cfg_* functions return success, ENODEV or ENODATA.
We want to stop only in case there is something wrong with the
provided data. A missing device is not an issue here. In this
case we want to keep the configuration as is and complete stuff
once a device is being attached.
>
> >
> > + pr_err("Config failed\n");
> > + return ret;
> > + }
> > + }
> > +
> > + return most_add_link(mdev_link->mdev, mdev_link->channel,
> > + mdev_link->comp, mdev_link->name,
> > + mdev_link->param);
> > +}
> > +
> > static ssize_t mdev_link_create_store(struct config_item *item,
> > const char *page, size_t
> > count)
> > {
> > struct mdev_link *mdev_link = to_mdev_link(item);
> > bool tmp;
> > int ret;
> > - int i;
> >
> > ret = kstrtobool(page, );
> > if (ret)
> > return ret;
> > -
> > - for (i = 0; i < ARRAY_SIZE(set_config_val); i++) {
> > - ret = set_config_val[i](mdev_link);
> > - if (ret < 0)
> > - return ret;
> > - }
> > -
> > - if (tmp)
> > - ret = most_add_link(mdev_link->mdev, mdev_link-
> > >channel,
> > - mdev_link->comp, mdev_link-
> > >name,
> > - mdev_link->param);
> > - else
> > - ret = most_remove_link(mdev_link->mdev, mdev_link-
> > >channel,
> > - mdev_link->comp);
> > - if (ret)
> > + if (!tmp)
> > + return most_remove_link(mdev_link->mdev,
> > mdev_link->channel,
> > + mdev_link->comp);
> > + ret = set_config_and_add_link(mdev_link);
> > + if (ret && ret != -ENODEV)
> ENODEV is success. I feel like this needs to be explained in
> comments
> in the code.
ENODEV is not a show-stopper. It only postpones the configuration
process until the driver is being notified about a new device.
>
> >
> > return ret;
> > + list_add_tail(_link->list, _link_list);
> > mdev_link->create = tmp;
> > return count;
> > }
> > @@ -590,6 +601,22 @@ int most_register_configfs_subsys(struct
> > core_component *c)
> > }
> > EXPORT_SYMBOL_GPL(most_register_configfs_subsys);
> >
> > +void most_interface_register_notify(const char *mdev)
> > +{
> > + bool register_snd_card = false;
> > + struct mdev_link *mdev_link;
> > +
> > + list_for_each_entry(mdev_link, _link_list, list) {
> > + if (!strcmp(mdev_link->mdev, mdev)) {
> > + set_config_and_add_link(mdev_link);
> Here the errors are not checked.
We are in the notify function. That means a device is present
now. And if the list isn't empty, there is also a valid configuration
available. So, set_config_and_add_link should not fail.
>
> >
> > + if (!strcmp(mdev_link->comp,
Re: [PATCH v4 0/6] staging: most: sound: change sound card layout
On Mon, 2018-12-17 at 17:16 +0300, Dan Carpenter wrote: > Thanks! > My fault. :) > Reviewed-by: Dan Carpenter > > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file
On Fr, 2018-12-14 at 12:10 +0300, Dan Carpenter wrote: > On Fri, Dec 14, 2018 at 09:06:01AM +0000, Christian.Gromm@microchip.c > om wrote: > > > > Message received. Anyway, I would like to resent the current > > patch set with the things Dan found fixed. And then looking > > into Configfs. Would this be ok for you guys? > > > Sure. > I'm not sure what is the best way to resend this. Can you skip the series and I'm going to send an entire new one. Or should I create a v2 of some of the patches of the series and only resend those and you keep the others? thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file
On Do, 2018-12-13 at 18:55 +0100, Greg KH wrote: > On Thu, Dec 13, 2018 at 04:32:25PM +0000, Christian.Gromm@microchip.c > om wrote: > > > > On Do, 2018-12-13 at 13:32 +0100, Greg KH wrote: > > > > > > On Thu, Dec 13, 2018 at 02:58:00PM +0300, Dan Carpenter wrote: > > > > > > > > > > > > On Wed, Dec 12, 2018 at 01:15:31PM +0100, Christian Gromm > > > > wrote: > > > > > > > > > > > > > > > This patch updates driver_usage.txt file to reflect the > > > > > latest > > > > > changes > > > > > that this patch set introduces. > > > > > > > > > > Signed-off-by: Christian Gromm > > > > > > > > > > --- > > > > > drivers/staging/most/Documentation/driver_usage.txt | 16 > > > > > +--- > > > > > 1 file changed, 13 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git > > > > > a/drivers/staging/most/Documentation/driver_usage.txt > > > > > b/drivers/staging/most/Documentation/driver_usage.txt > > > > > index bb9b4e8..da7a8f4 100644 > > > > > --- a/drivers/staging/most/Documentation/driver_usage.txt > > > > > +++ b/drivers/staging/most/Documentation/driver_usage.txt > > > > > @@ -142,8 +142,9 @@ Cdev component example: > > > > > > > > > > Sound component example: > > > > > > > > > > -The sound component needs an additional parameter to > > > > > determine > > > > > the audio > > > > > -resolution that is going to be used. The following formats > > > > > are > > > > > available: > > > > > +The sound component needs additional parameters to determine > > > > > the > > > > > audio > > > > > +resolution that is going to be used and to trigger the > > > > > registration of a > > > > > +sound card with ALSA. The following audio formats are > > > > > available: > > > > > > > > > > - "1x8" (Mono) > > > > > - "2x16" (16-bit stereo) > > > > > @@ -151,9 +152,18 @@ resolution that is going to be used. The > > > > > following formats are available: > > > > > - "2x32" (32-bit stereo) > > > > > - "6x16" (16-bit surround 5.1) > > > > > > > > > > -$ echo "mdev0:ep_81:sound:most51_playback.6x16" > > > > > > > > > > > > $(DRV_DIR)/add_link > > > > > +To make the sound module create a sound card and register it > > > > > with ALSA the > > > > > +string "create" needs to be attached to the module parameter > > > > > section of the > > > > > +configuration string. To create a sound card with with two > > > > > playback devices > > > > > +(linked to channel ep01 and ep02) and one capture device > > > > > (linked > > > > > to channel > > > > > +ep83) the following is written to the add_link file: > > > > > > > > > > +$ echo "mdev0:ep01:sound:most51_playback.6x16" > > > > > > > > > > > > $(DRV_DIR)/add_link > > > > > +$ echo "mdev0:ep02:sound:most_playback.2x16" > > > > > > > > > > > > $(DRV_DIR)/add_link > > > > > +$ echo "mdev0:ep83:sound:most_capture.2x16.create" > > > > > > > > > > > > $(DRV_DIR)/add_link > > > > I would maybe prefer if the "create" command were on a separate > > > > line. > > > > Something like: > > > > > > > > +$ echo "mdev0:ep01:sound:most51_playback.6x16" > > > > > > > > > > $(DRV_DIR)/add_link > > > > +$ echo "mdev0:ep02:sound:most_playback.2x16" > > > > > > > > > > $(DRV_DIR)/add_link > > > > +$ echo "mdev0:ep83:sound:most_capture.2x16" > > > > > > > > > > $(DRV_DIR)/add_link > > > > +$ echo "mdev0:ep83:sound:create" > > > > >$(DRV_DIR)/sound_card > > > > > > > > It's sort of a separate command in a way? > > > Why is sysfs being used for configuring things? Isn't that what > > > configfs was created for? :) > > > > > Omg, that would be one API change! > > > > I need to dig a little deeper into configfs to recognize > > its beauty and the possible benefit for our driver. > > > > Do you see this as a prerequisite? > Writing random strings to a random sysfs file to configure things is > not > a "normal" user/kernel api as sysfs files are supposed to just be > "one > value" and not parsed like what you are doing here. > > So I would strongly suggest looking at configfs, that is what it was > designed for. > Message received. Anyway, I would like to resent the current patch set with the things Dan found fixed. And then looking into Configfs. Would this be ok for you guys? thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file
On Do, 2018-12-13 at 13:32 +0100, Greg KH wrote: > On Thu, Dec 13, 2018 at 02:58:00PM +0300, Dan Carpenter wrote: > > > > On Wed, Dec 12, 2018 at 01:15:31PM +0100, Christian Gromm wrote: > > > > > > This patch updates driver_usage.txt file to reflect the latest > > > changes > > > that this patch set introduces. > > > > > > Signed-off-by: Christian Gromm > > > --- > > > drivers/staging/most/Documentation/driver_usage.txt | 16 > > > +--- > > > 1 file changed, 13 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/staging/most/Documentation/driver_usage.txt > > > b/drivers/staging/most/Documentation/driver_usage.txt > > > index bb9b4e8..da7a8f4 100644 > > > --- a/drivers/staging/most/Documentation/driver_usage.txt > > > +++ b/drivers/staging/most/Documentation/driver_usage.txt > > > @@ -142,8 +142,9 @@ Cdev component example: > > > > > > Sound component example: > > > > > > -The sound component needs an additional parameter to determine > > > the audio > > > -resolution that is going to be used. The following formats are > > > available: > > > +The sound component needs additional parameters to determine the > > > audio > > > +resolution that is going to be used and to trigger the > > > registration of a > > > +sound card with ALSA. The following audio formats are available: > > > > > > - "1x8" (Mono) > > > - "2x16" (16-bit stereo) > > > @@ -151,9 +152,18 @@ resolution that is going to be used. The > > > following formats are available: > > > - "2x32" (32-bit stereo) > > > - "6x16" (16-bit surround 5.1) > > > > > > -$ echo "mdev0:ep_81:sound:most51_playback.6x16" > > > >$(DRV_DIR)/add_link > > > +To make the sound module create a sound card and register it > > > with ALSA the > > > +string "create" needs to be attached to the module parameter > > > section of the > > > +configuration string. To create a sound card with with two > > > playback devices > > > +(linked to channel ep01 and ep02) and one capture device (linked > > > to channel > > > +ep83) the following is written to the add_link file: > > > > > > +$ echo "mdev0:ep01:sound:most51_playback.6x16" > > > >$(DRV_DIR)/add_link > > > +$ echo "mdev0:ep02:sound:most_playback.2x16" > > > >$(DRV_DIR)/add_link > > > +$ echo "mdev0:ep83:sound:most_capture.2x16.create" > > > >$(DRV_DIR)/add_link > > I would maybe prefer if the "create" command were on a separate > > line. > > Something like: > > > > +$ echo "mdev0:ep01:sound:most51_playback.6x16" > > >$(DRV_DIR)/add_link > > +$ echo "mdev0:ep02:sound:most_playback.2x16" > > >$(DRV_DIR)/add_link > > +$ echo "mdev0:ep83:sound:most_capture.2x16" > > >$(DRV_DIR)/add_link > > +$ echo "mdev0:ep83:sound:create" >$(DRV_DIR)/sound_card > > > > It's sort of a separate command in a way? > Why is sysfs being used for configuring things? Isn't that what > configfs was created for? :) > Omg, that would be one API change! I need to dig a little deeper into configfs to recognize its beauty and the possible benefit for our driver. Do you see this as a prerequisite? thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file
On Do, 2018-12-13 at 14:54 +0300, Dan Carpenter wrote: > I'm not really complaining about breaking userspace, I'm complaining > that I had to discover it by reading the code. It should have been > mentioned in the commit message. We should probably just fold > patch 1 & 6 together. > Ok, I probably should have mentioned this in the cover letter. Thought the usage file addition is enough. Sorry for the inconvenience. > I'm also not really complaining about this patch in particular when > it > comes to the API. The whole thing is a bit weird to me. I wish we > could re-think the configuration from square one... > > It could be done in a later patch. I'm not going to NAK this patch > if > you resend it with the other small issues fixed. > Greg put configfs on the table. But this is like a total redesign of the driver interface which will take some time. thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/6] staging: most: sound: create one sound card w/ multiple PCM devices per MOST device
On Do, 2018-12-13 at 15:38 +0300, Dan Carpenter wrote: > On Wed, Dec 12, 2018 at 03:31:13PM +0000, Christian.Gromm@microchip.c > om wrote: > > > > An additional field is added to the configuration parameter, > > which is provided by user space. > > This seemed to be less painful than adding a new sysfs > > file and make the configuration even more complicated. > I think that adding a bunch of new sysfs files *and directories* is > the > way to go actually. > > echo "mdev0:ep01:sound:most51_playback.6x16" >$(DRV_DIR)/add_link > > I've never used the most driver but my guess is that $DRV_DIR is > /sys/module/most/. In theory, you're supposed to write one word or > number to sysfs files so this is sort of a misuse of the API. > Well, you have to have a MOST or INICnet Infotainment network on your desk to use the driver. Actually, the $DRV_DIR is /sys/bus/most/drivers/most_core. And if the driver is loaded and a MOST device is attached that features two channels we get: /sys/bus/most/ ├── devices │ └── mdev0 -> ../../../devices/most_bus/mdev0 ├── drivers │ └── most_core │ ├── add_link │ ├── bind │ ├── components │ ├── links │ ├── mdev0 -> ../../../../devices/most_bus/mdev0 │ ├── remove_link │ ├── uevent │ └── unbind ├── drivers_autoprobe ├── drivers_probe └── uevent Having a new default attr under $DRV_DIR that is exclusively used for the sound module is not really nice, unless it appears only in case the sound module itself is loaded. But as I said, I'm not sure if we can establish that with the current struct core_component as it lacks the struct device. thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/6] staging: most: sound: create one sound card w/ multiple PCM devices per MOST device
On Do, 2018-12-13 at 15:16 +0300, Dan Carpenter wrote:
> On Wed, Dec 12, 2018 at 01:15:26PM +0100, Christian Gromm wrote:
> >
> > @@ -571,6 +600,40 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> > return -EINVAL;
> > }
> >
> > + ret = split_arg_list(arg_list, _name, _num,
> > _res,
> > + );
> > + if (ret < 0)
> > + return ret;
> > +
> > + list_for_each_entry(adpt, _list, list) {
> > + if (adpt->iface == iface && adpt->registered)
> > + return -ENOSPC;
> > + if (!adpt->registered) {
> > + adpt->pcm_dev_idx++;
> > + goto skip_adpt_alloc;
> We haven't ensured the adpt->iface == iface.
>
> >
> > + }
> > + }
> Probably you want to say:
>
> list_for_each_entry(adpt, _list, list) {
> if (adpt->iface != iface)
> continue;
> if (adpt->registered)
> return -ENOSPC;
> adpt->pcm_dev_idx++;
> goto skip_adpt_alloc;
> }
>
I do.
> But here again, I think I might prefer if allocating a new "adpt"
> were
> and explicit command from user space as opposed to just a side effect
> of
> registering a new iface.
>
Sounds reasonable. But, problem here is that we want the
process of how channels are linked to be independent from the
component that is being used. That's when things start to
become complicated.
In other words, I don't want the sound module to be configured
in a different way than the cdev module, networking module or the
v4l2 module is. The goal was to have only sysfs files that
are generic (->default attrs of most_core mod) and apply to all
components.
The latest change of the sound module creates the need of
signaling that the user is done with the config of the ALSA card.
The quickest way to achieve this, was to expand the "module
parameter" field (which we already have) with the "create" flag
that is passed to the module when a channel is being probed as a
trigger.
Because no other module needs such a trigger, creating a dedicated
sysfs file for this purpose seemed kind of weird, especially when
the sound module isn't loaded at all.
Maybe the sound module should create its own file, but since it
has no struct device embedded, I'm not sure how to achieve that :/
thanks,
Chris
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/6] staging: most: Documentation: add information to driver_usage file
On Mi, 2018-12-12 at 17:31 +0300, Dan Carpenter wrote: > On Wed, Dec 12, 2018 at 01:15:31PM +0100, Christian Gromm wrote: > > > > diff --git a/drivers/staging/most/Documentation/driver_usage.txt > > b/drivers/staging/most/Documentation/driver_usage.txt > > index bb9b4e8..da7a8f4 100644 > > --- a/drivers/staging/most/Documentation/driver_usage.txt > > +++ b/drivers/staging/most/Documentation/driver_usage.txt > > @@ -142,8 +142,9 @@ Cdev component example: > > > > Sound component example: > > > > -The sound component needs an additional parameter to determine the > > audio > > -resolution that is going to be used. The following formats are > > available: > > +The sound component needs additional parameters to determine the > > audio > > +resolution that is going to be used and to trigger the > > registration of a > > +sound card with ALSA. The following audio formats are available: > > > > - "1x8" (Mono) > > - "2x16" (16-bit stereo) > > @@ -151,9 +152,18 @@ resolution that is going to be used. The > > following formats are available: > > - "2x32" (32-bit stereo) > > - "6x16" (16-bit surround 5.1) > > > > -$ echo "mdev0:ep_81:sound:most51_playback.6x16" > > >$(DRV_DIR)/add_link > > +To make the sound module create a sound card and register it with > > ALSA the > > +string "create" needs to be attached to the module parameter > > section of the > > +configuration string. To create a sound card with with two > > playback devices > > +(linked to channel ep01 and ep02) and one capture device (linked > > to channel > > +ep83) the following is written to the add_link file: > > > > +$ echo "mdev0:ep01:sound:most51_playback.6x16" > > >$(DRV_DIR)/add_link > > +$ echo "mdev0:ep02:sound:most_playback.2x16" > > >$(DRV_DIR)/add_link > > +$ echo "mdev0:ep83:sound:most_capture.2x16.create" > > >$(DRV_DIR)/add_link > > > > +The link names (most51_playback, most_playback and most_capture) > > will > > +become the names of the PCM devices of the sound card. > So this patchset does break userspace... Which is allowed sometimes > in > staging, but it's better to point it out in the original commit which > causes the breakage. > The sound card layout that we have in mind is, from a system architecture point of view, the better one. Unfortunately, it does come with the cost of changing the configuration in one way or the other. Which is not really great, I agree on that. What I don't see is, why enhancing the way how user space interferes wi th the driver is such a terrible thing to do in staging. This should be the place to be to develop and change things (unless I totally misunderstood the concept behind it). > But really I don't like this API at all... It feels like something Fair enough. Is it just latest change that made you feel this way, or didn't you like the process in the first place? > from decades ago. There has to be a better way than this. > What if I would find a better way and change the API into something you like. Wouldn't that break user space too? Is this a dead end? How am I supposed to go on? > Unfortunately, I'm not clever enough to give you useful > suggestions... > thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/6] staging: most: sound: rename variable
On Mi, 2018-12-12 at 17:26 +0300, Dan Carpenter wrote: > On Wed, Dec 12, 2018 at 01:15:28PM +0100, Christian Gromm wrote: > > > > @@ -587,7 +587,7 @@ static int audio_probe_channel(struct > > most_interface *iface, int channel_id, > > int capture_count = 0; > > int ret; > > int direction; > > - char *card_name; > > + char *device_name; > ^ > > > > > u16 ch_num; > > u8 create = 0; > > char *sample_res; > > @@ -600,7 +600,7 @@ static int audio_probe_channel(struct > > most_interface *iface, int channel_id, > > return -EINVAL; > > } > > > > - ret = split_arg_list(arg_list, _name, _num, > > _res, > > + ret = split_arg_list(arg_list, _name, _num, > > _res, > > ); > > if (ret < 0) > > return ret; > > @@ -622,7 +622,7 @@ static int audio_probe_channel(struct > > most_interface *iface, int channel_id, > > adpt->pcm_dev_idx = 0; > > INIT_LIST_HEAD(>dev_list); > > iface->priv = adpt; > > - ret = snd_card_new(>dev, -1, card_name, > > THIS_MODULE, > > + ret = snd_card_new(>dev, -1, device_name, > > THIS_MODULE, > > sizeof(*channel), >card); > > if (ret < 0) > > return ret; > > @@ -663,14 +663,14 @@ static int audio_probe_channel(struct > > most_interface *iface, int channel_id, > > if (ret) > > goto err_free_adpt; > > > > - ret = snd_pcm_new(adpt->card, card_name, adpt- > > >pcm_dev_idx, > > + ret = snd_pcm_new(adpt->card, device_name, adpt- > > >pcm_dev_idx, > > playback_count, capture_count, ); > > > > if (ret < 0) > > goto err_free_adpt; > > > > pcm->private_data = channel; > > - > > + snprintf(pcm->name, sizeof(device_name), device_name); > ^^^ > > This change was not described in the commit message and it's not > correct. sizeof(device_name) is sizeof(long). We should be taking > sizeof(pcm->name) which is 80. > Right, this needs to be a separate patch. Missed that one while interactively staging the individual changes. thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/6] staging: most: sound: create one sound card w/ multiple PCM devices per MOST device
On Mi, 2018-12-12 at 17:21 +0300, Dan Carpenter wrote:
> On Wed, Dec 12, 2018 at 01:15:26PM +0100, Christian Gromm wrote:
> >
> > This patch avoids that a sound card is created and registered with
> > ALSA
> > every time a channel is being linked. Instead the channels are
> > hooked on
> > the same card, which is registered not until the final link has
> > been added
> > to the component.
> >
> > Signed-off-by: Christian Gromm
> > ---
> > drivers/staging/most/sound/sound.c | 127
> > +
> > 1 file changed, 87 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/staging/most/sound/sound.c
> > b/drivers/staging/most/sound/sound.c
> > index 89b02fc..41bcd2c 100644
> > --- a/drivers/staging/most/sound/sound.c
> > +++ b/drivers/staging/most/sound/sound.c
> > @@ -10,6 +10,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -20,7 +21,6 @@
> >
> > #define DRIVER_NAME "sound"
> >
> > -static struct list_head dev_list;
> > static struct core_component comp;
> >
> > /**
> > @@ -56,6 +56,17 @@ struct channel {
> > void (*copy_fn)(void *alsa, void *most, unsigned int
> > bytes);
> > };
> >
> > +struct sound_adapter {
> > + struct list_head dev_list;
> > + struct most_interface *iface;
> > + struct snd_card *card;
> > + struct list_head list;
> > + bool registered;
> > + int pcm_dev_idx;
> > +};
> > +
> > +static struct list_head adpt_list;
> > +
> > #define MOST_PCM_INFO (SNDRV_PCM_INFO_MMAP | \
> > SNDRV_PCM_INFO_MMAP_VALID | \
> > SNDRV_PCM_INFO_BATCH | \
> > @@ -157,9 +168,10 @@ static void most_to_alsa_copy32(void *alsa,
> > void *most, unsigned int bytes)
> > static struct channel *get_channel(struct most_interface *iface,
> > int channel_id)
> > {
> > + struct sound_adapter *adpt = iface->priv;
> > struct channel *channel, *tmp;
> >
> > - list_for_each_entry_safe(channel, tmp, _list, list) {
> > + list_for_each_entry_safe(channel, tmp, >dev_list,
> > list) {
> > if ((channel->iface == iface) && (channel->id ==
> > channel_id))
> > return channel;
> > }
> > @@ -460,7 +472,7 @@ static const struct snd_pcm_ops pcm_ops = {
> > };
> >
> > static int split_arg_list(char *buf, char **card_name, u16
> > *ch_num,
> > - char **sample_res)
> > + char **sample_res, u8 *create)
> > {
> > char *num;
> > int ret;
> > @@ -479,6 +491,9 @@ static int split_arg_list(char *buf, char
> > **card_name, u16 *ch_num,
> > *sample_res = strsep(, ".\n");
> > if (!*sample_res)
> > goto err;
> > +
> > + if (buf && !strcmp(buf, "create"))
> > + *create = 1;
> This comes from userspace, right? So we're adding a new API?
>
An additional field is added to the configuration parameter,
which is provided by user space.
This seemed to be less painful than adding a new sysfs
file and make the configuration even more complicated.
> >
> > return 0;
> >
> > err:
> > @@ -536,6 +551,19 @@ static int audio_set_hw_params(struct
> > snd_pcm_hardware *pcm_hw,
> > return 0;
> > }
> >
> > +static void release_adapter(struct sound_adapter *adpt)
> > +{
> > + struct channel *channel, *tmp;
> > +
> > + list_for_each_entry_safe(channel, tmp, >dev_list,
> > list) {
> > + list_del(>list);
> > + kfree(channel);
> > + }
> > + snd_card_free(adpt->card);
> > + list_del(>list);
> > + kfree(adpt);
> > +}
> > +
> > /**
> > * audio_probe_channel - probe function of the driver module
> > * @iface: pointer to interface instance
> > @@ -553,7 +581,7 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> > char *arg_list)
> > {
> > struct channel *channel;
> > - struct snd_card *card;
> > + struct sound_adapter *adpt;
> > struct snd_pcm *pcm;
> > int playback_count = 0;
> > int capture_count = 0;
> > @@ -561,6 +589,7 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> > int direction;
> > char *card_name;
> > u16 ch_num;
> > + u8 create = 0;
> > char *sample_res;
> >
> > if (!iface)
> > @@ -571,6 +600,40 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> > return -EINVAL;
> > }
> >
> > + ret = split_arg_list(arg_list, _name, _num,
> > _res,
> > + );
> > + if (ret < 0)
> > j+ return ret;
> > +
> > + list_for_each_entry(adpt, _list, list) {
> > + if (adpt->iface == iface && adpt->registered)
> > + return -ENOSPC;
> > + if (!adpt->registered) {
> This is very confusing and I'm sorry but I don't think it even
> works...
>
> :(
>
> We add new allocations to the _list, but then if
> audio_probe_channel()
> fails, then we free
