Re: [PATCH/RFC 1/2] ALSA: fireface: Fix integer overflow in transmit_midi_msg()
Hi,
On Mon, Jan 11, 2021 at 02:02:50PM +0100, Geert Uytterhoeven wrote:
> As snd_ff.rx_bytes[] is unsigned int, and NSEC_PER_SEC is 10L,
> the second multiplication in
>
> ff->rx_bytes[port] * 8 * NSEC_PER_SEC / 31250
>
> always overflows on 32-bit platforms, truncating the result. Fix this
> by precalculating "NSEC_PER_SEC / 31250", which is an integer constant.
>
> Note that this assumes ff->rx_bytes[port] <= 16777.
>
> Fixes: 19174295788de77d ("ALSA: fireface: add transaction support")
> Signed-off-by: Geert Uytterhoeven
> ---
> Compile-tested only.
>
> I don't know the maximum transfer length of MIDI, but given it's an old
> standard, I guess it's rather small. If it is larger than 16777, the
> constant "8" should be replaced by "8ULL", to force 64-bit arithmetic.
> ---
> sound/firewire/fireface/ff-transaction.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
The rx_bytes member has value for the length of byte messages to
process. The range of value differs depending on Fireface protocol
version. For former protocol, the value is equals to or less than
SND_FF_MAXIMIM_MIDI_QUADS (= 9). For latter protocol, the value is
equals to or less than 3. Anyway, the value should not be larger
than 16777 and the calculation can be done without ULL suffix.
Reviewd-by: Takashi Sakamoto
> diff --git a/sound/firewire/fireface/ff-transaction.c
> b/sound/firewire/fireface/ff-transaction.c
> index 7f82762ccc8c80ba..ee7122c461d46f44 100644
> --- a/sound/firewire/fireface/ff-transaction.c
> +++ b/sound/firewire/fireface/ff-transaction.c
> @@ -88,7 +88,7 @@ static void transmit_midi_msg(struct snd_ff *ff, unsigned
> int port)
>
> /* Set interval to next transaction. */
> ff->next_ktime[port] = ktime_add_ns(ktime_get(),
> - ff->rx_bytes[port] * 8 * NSEC_PER_SEC / 31250);
> + ff->rx_bytes[port] * 8 * (NSEC_PER_SEC / 31250));
>
> if (quad_count == 1)
> tcode = TCODE_WRITE_QUADLET_REQUEST;
> --
> 2.25.1
Thanks
Takashi Sakamoto
Re: [PATCH/RFC 2/2] ALSA: firewire-tascam: Fix integer overflow in midi_port_work()
Hi,
On Mon, Jan 11, 2021 at 02:02:51PM +0100, Geert Uytterhoeven wrote:
> As snd_fw_async_midi_port.consume_bytes is unsigned int, and
> NSEC_PER_SEC is 10L, the second multiplication in
>
> port->consume_bytes * 8 * NSEC_PER_SEC / 31250
>
> always overflows on 32-bit platforms, truncating the result. Fix this
> by precalculating "NSEC_PER_SEC / 31250", which is an integer constant.
>
> Note that this assumes port->consume_bytes <= 16777.
>
> Fixes: 531f471834227d03 ("ALSA: firewire-lib/firewire-tascam: localize async
> midi port")
> Signed-off-by: Geert Uytterhoeven
> ---
> Compile-tested only.
>
> I don't know the maximum transfer length of MIDI, but given it's an old
> standard, I guess it's rather small. If it is larger than 16777, the
> constant "8" should be replaced by "8ULL", to force 64-bit arithmetic.
> ---
> sound/firewire/tascam/tascam-transaction.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Indeed. The calculation brings integer overflow of 32 bit storage. Thanks
for your care and proposal of the patch. I agree with the intension of
patch, however I have a nitpicking that the consume_bytes member is
defined as 'int', not 'unsigned int' in your commit comment.
The member has value returned from the call of 'fill_message()'[1] for the
length of byte messages in buffer to process, or for error code. The
error code is checked immediately. The range of value is equal to
or less than 3 when reaching the calculation, thus it should be less than
16777.
Regardless of the type of 'int' or 'unsigned int', this patch can fix
the issued problem. Feel free to add my tag when you post second version
with comment fix.
Reviewed-by: Takashi Sakamoto
> diff --git a/sound/firewire/tascam/tascam-transaction.c
> b/sound/firewire/tascam/tascam-transaction.c
> index 90288b4b46379526..a073cece4a7d5e3a 100644
> --- a/sound/firewire/tascam/tascam-transaction.c
> +++ b/sound/firewire/tascam/tascam-transaction.c
> @@ -209,7 +209,7 @@ static void midi_port_work(struct work_struct *work)
>
> /* Set interval to next transaction. */
> port->next_ktime = ktime_add_ns(ktime_get(),
> - port->consume_bytes * 8 * NSEC_PER_SEC / 31250);
> + port->consume_bytes * 8 * (NSEC_PER_SEC / 31250));
>
> /* Start this transaction. */
> port->idling = false;
> --
> 2.25.1
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/sound/firewire/tascam/tascam-transaction.c#n197
Thanks
Takashi Sakamoto
Re: [PATCH] ALSA: firewire: fix comparison to bool warning
On Tue, Nov 10, 2020 at 09:06:29AM +0100, Takashi Iwai wrote:
> On Sat, 07 Nov 2020 17:13:31 +0100,
> xiakaixu1...@gmail.com wrote:
> >
> > From: Kaixu Xia
> >
> > Fix the following coccicheck warning:
> >
> > ./sound/firewire/amdtp-stream.h:273:6-19: WARNING: Comparison to bool
> >
> > Reported-by: Tosk Robot
> > Signed-off-by: Kaixu Xia
The message seems not to reach my mailbox. Anyway:
Acked-by: Takashi Sakamoto
> Thanks, applied.
>
>
> Takashi
>
> > ---
> > sound/firewire/amdtp-stream.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sound/firewire/amdtp-stream.h b/sound/firewire/amdtp-stream.h
> > index 2ceb57d1d58e..a3daa1f2c1c4 100644
> > --- a/sound/firewire/amdtp-stream.h
> > +++ b/sound/firewire/amdtp-stream.h
> > @@ -270,7 +270,7 @@ static inline bool amdtp_stream_wait_callback(struct
> > amdtp_stream *s,
> > unsigned int timeout)
> > {
> > return wait_event_timeout(s->callback_wait,
> > - s->callbacked == true,
> > + s->callbacked,
> > msecs_to_jiffies(timeout)) > 0;
> > }
> >
> > --
> > 2.20.0
Regards
Takashi Sakamoto
Re: [PATCH] firewire: fix function type cast warning
Hi Arnd,
On Mon, Oct 26, 2020 at 10:51:27PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> gcc -Wextra complains about a suspicious cast:
>
> rivers/firewire/core-cdev.c:985:8: warning: cast between incompatible
> function types from ‘void (*)(struct fw_iso_context *, dma_addr_t, void *)’
> {aka ‘void (*)(struct fw_iso_context *, long long unsigned int, void *)’} to
> ‘void (*)(struct fw_iso_context *, u32, size_t, void *, void *)’ {aka ‘void
> (*)(struct fw_iso_context *, unsigned int, long unsigned int, void *, void
> *)’} [-Wcast-function-type]
>
> The behavior is correct in the end, but this is more clearly
> expressed using a transparent union.
>
> Fixes: 872e330e3880 ("firewire: add isochronous multichannel reception")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/firewire/core-cdev.c | 6 +++---
> drivers/firewire/core-iso.c | 2 +-
> include/linux/firewire.h | 17 -
> 3 files changed, 12 insertions(+), 13 deletions(-)
Oscar Carter has posted a patch to fix it.
https://sourceforge.net/p/linux1394/mailman/message/37024966/
I don't know exactly but maintainers seems to overlook it...
Thanks
Takashi Sakamoto
Re: [PATCH 4/8] ALSA: fireworks: use semicolons rather than commas to separate statements
Hi,
On Sun, Oct 11, 2020 at 11:19:35AM +0200, Julia Lawall wrote:
> Replace commas with semicolons. What is done is essentially described by
> the following Coccinelle semantic patch (http://coccinelle.lip6.fr/):
>
> //
> @@ expression e1,e2; @@
> e1
> -,
> +;
> e2
> ... when any
> //
>
> Signed-off-by: Julia Lawall
>
> ---
> sound/firewire/fireworks/fireworks_pcm.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/firewire/fireworks/fireworks_pcm.c
> b/sound/firewire/fireworks/fireworks_pcm.c
> index 980580dfbb39..a0d5db1d8eb2 100644
> --- a/sound/firewire/fireworks/fireworks_pcm.c
> +++ b/sound/firewire/fireworks/fireworks_pcm.c
> @@ -148,7 +148,7 @@ pcm_init_hw_params(struct snd_efw *efw,
> }
>
> /* limit rates */
> - runtime->hw.rates = efw->supported_sampling_rate,
> + runtime->hw.rates = efw->supported_sampling_rate;
> snd_pcm_limit_hw_rates(runtime);
>
> limit_channels(>hw, pcm_channels);
Oops. It seems to be my typo added at the commit aa02bb6e6078
("ALSA: fireworks: Add PCM interface")...
Acked-by: Takashi Sakamoto
Thanks
Takashi Sakamoto
Re: [PATCH v3] firewire: Remove function callback casts
Hi, I'm sorry to be late but I was stuck at my work for ALSA control service programs for audio and music units on IEEE 1394 bus[1]. On Sat, May 30, 2020 at 11:08:39AM +0200, Oscar Carter wrote: > In 1394 OHCI specification, Isochronous Receive DMA context has several > modes. One of mode is 'BufferFill' and Linux FireWire stack uses it to > receive isochronous packets for multiple isochronous channel as > FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL. > > The mode is not used by in-kernel driver, while it's available for > userspace. The character device driver in firewire-core includes > cast of function callback for the mode since the type of callback > function is different from the other modes. The case is inconvenient > to effort of Control Flow Integrity builds due to > -Wcast-function-type warning. > > This commit removes the cast. A static helper function is newly added > to initialize isochronous context for the mode. The helper function > arranges isochronous context to assign specific callback function > after call of existent kernel API. It's noticeable that the number of > isochronous channel, speed, and the size of header are not required for > the mode. The helper function is used for the mode by character device > driver instead of direct call of existent kernel API. > > The same goal can be achieved (in the ioctl_create_iso_context function) > without this helper function as follows: > - Call the fw_iso_context_create function passing NULL to the callback > parameter. > - Then setting the context->callback.sc or context->callback.mc > variables based on the a->type value. > > However using the helper function created in this patch makes code more > clear and declarative. This way avoid the call to a function with one > purpose to achieved another one. > > Co-developed-by: Takashi Sakamoto > Signed-off-by: Takashi Sakamoto > Co-developed-by: Stefan Richter > Signed-off-by: Stefan Richter > Signed-off-by: Oscar Carter > --- > Hi, > > this is another proposal to achieved the goal of remove function callback > cast start by me with the first [1] and second [2] versions, and followed > by the work of Takashi Sakamoto with his first [3] and second [4] versions, > and the code of Stefan Richter [5]. > > The purpose of this third version is to put together all the work done > until now following the comments of all reviewed patches. > > I've added the "Co-developed-by" and "Signed-off-by" tags to give credit to > Takashi Sakamoto and Stefan Richter if there are no objections. In my opinion, it's no need to add my and Stefan's sign-off tag to patch in which you firstly wrote even if it includes ideas from the others ;) > Changelog v1->v2 > -Set explicity to NULL the "ctx->callback.sc" variable and return an error > code in "fw_iso_context_create" function if both callback parameters are > NULL as Lev R. Oshvang suggested. > -Modify the commit changelog accordingly. > > Changelog v2->v3 > -Put togeher all the work done in different patches by different authors. > -Modify the previous work following the comments in the reviewed patches. > > [1] https://lore.kernel.org/lkml/20200516173934.31527-1-oscar.car...@gmx.com/ > [2] https://lore.kernel.org/lkml/20200519173425.4724-1-oscar.car...@gmx.com/ > [3] > https://lore.kernel.org/lkml/20200520064726.31838-1-o-taka...@sakamocchi.jp/ > [4] > https://lore.kernel.org/lkml/20200524132048.243223-1-o-taka...@sakamocchi.jp/ > [5] https://lore.kernel.org/lkml/20200525015532.0055f9df@kant/ > > drivers/firewire/core-cdev.c | 32 ++++++++++-- > include/linux/firewire.h | 11 +++ > 2 files changed, 33 insertions(+), 10 deletions(-) Anyway this patch looks good to me. I test this patch with libhinoko and find no regression. Reviewed-by: Takashi Sakamoto Testeb-by: Takashi Sakamoto [1] [RFT] ALSA control service programs for Digidesign Digi 002/003 family and Tascam FireWire series https://mailman.alsa-project.org/pipermail/alsa-devel/2020-July/170331.html Thanks Takashi Sakamoto
Re: [PATCH] ANDROID: sound: usb: Add vendor's hooking interface
Hi,
On Wed, Jun 17, 2020 at 11:18:24AM +0900, JaeHun Jung wrote:
> In mobile, a co-processor is used when using USB audio
> to improve power consumption.
> hooking is required for sync-up when operating
> the co-processor. So register call-back function.
> The main operation of the call-back function is as follows:
> - Initialize the co-processor by transmitting data
> when initializing.
> - Change the co-processor setting value through
> the interface function.
> - Configure sampling rate
> - pcm open/close
>
> Bug: 156315379
>
> Change-Id: I32e1dd408e64aaef68ee06c480c4b4d4c95546dc
> Signed-off-by: JaeHun Jung
> ---
> sound/usb/card.c | 16
> sound/usb/card.h | 1 +
> sound/usb/clock.c| 5 +
> sound/usb/pcm.c | 33 +
> sound/usb/usbaudio.h | 30 ++
> 5 files changed, 85 insertions(+)
> diff --git a/sound/usb/card.c b/sound/usb/card.c
> index fd6fd17..2f3fa14 100644
> --- a/sound/usb/card.c
> +++ b/sound/usb/card.c
> @@ -111,6 +111,7 @@ MODULE_PARM_DESC(skip_validation, "Skip unit descriptor
> validation (default: no)
> static DEFINE_MUTEX(register_mutex);
> static struct snd_usb_audio *usb_chip[SNDRV_CARDS];
> static struct usb_driver usb_audio_driver;
> +struct snd_usb_audio_vendor_ops *usb_audio_ops;
>
> /*
> * disconnect streams
> @@ -210,6 +211,12 @@ static int snd_usb_create_stream(struct snd_usb_audio
> *chip, int ctrlif, int int
> return 0;
> }
>
> +void snd_set_vender_interface(struct snd_usb_audio_vendor_ops *vendor_ops)
> +{
> + usb_audio_ops = vendor_ops;
> +}
> +EXPORT_SYMBOL_GPL(snd_set_vender_interface);
I think the symbol name has typo; 'vender' against 'vendor'.
Anyway, this feature is not widely used at present. I suggest to add
kernel configuration for the feature at kernel compilation time, IMO.
Regards
Takashi Sakamoto
Re: [PATCH v2] firewire-core: remove cast of function callback
Hi,
On Mon, May 25, 2020 at 01:55:32AM +0200, Stefan Richter wrote:
> > Why is this in a .h file? What's wrong with just putting it in the .c
> > file as a static function? There's no need to make this an inline,
> > right?
>
> There is no need for this in a header.
> Furthermore, I prefer the original switch statement over the nested if/else.
>
> Here is another proposal; I will resend it later as a proper patch.
>
> diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
> index 719791819c24..bece1b69b43f 100644
> --- a/drivers/firewire/core-cdev.c
> +++ b/drivers/firewire/core-cdev.c
> @@ -957,7 +957,6 @@ static int ioctl_create_iso_context(struct client
> *client, union ioctl_arg *arg)
> {
> struct fw_cdev_create_iso_context *a = >create_iso_context;
> struct fw_iso_context *context;
> - fw_iso_callback_t cb;
> int ret;
>
> BUILD_BUG_ON(FW_CDEV_ISO_CONTEXT_TRANSMIT != FW_ISO_CONTEXT_TRANSMIT ||
> @@ -969,20 +968,15 @@ static int ioctl_create_iso_context(struct client
> *client, union ioctl_arg *arg)
> case FW_ISO_CONTEXT_TRANSMIT:
> if (a->speed > SCODE_3200 || a->channel > 63)
> return -EINVAL;
> -
> - cb = iso_callback;
> break;
>
> case FW_ISO_CONTEXT_RECEIVE:
> if (a->header_size < 4 || (a->header_size & 3) ||
> a->channel > 63)
> return -EINVAL;
> -
> - cb = iso_callback;
> break;
>
> case FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL:
> - cb = (fw_iso_callback_t)iso_mc_callback;
> break;
>
> default:
> @@ -990,9 +984,15 @@ static int ioctl_create_iso_context(struct client
> *client, union ioctl_arg *arg)
> }
>
> context = fw_iso_context_create(client->device->card, a->type,
> - a->channel, a->speed, a->header_size, cb, client);
> + a->channel, a->speed, a->header_size, NULL, client);
> if (IS_ERR(context))
> return PTR_ERR(context);
> +
> + if (a->type == FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL)
> + context->callback.mc = iso_mc_callback;
> + else
> + context->callback.sc = iso_callback;
> +
> if (client->version < FW_CDEV_VERSION_AUTO_FLUSH_ISO_OVERFLOW)
> context->drop_overflow_headers = true;
At first place, I wrote the similar patch but judged it's a bit ad-hoc
way that callback functions are assigned after the call of
fw_iso_context_create() in raw code. For explicitness in a point of things
being declarative, I put the inline function into header, and avoid
someone's misfortune for future even if IEEE 1394 is enough legacy.
Anyway, I don't mind Stefan's proposal since it works well. It depends
on developers' fashion to choose policy to write codes.
Thanks
Takashi Sakamoto
[PATCH v2] firewire-core: remove cast of function callback
In 1394 OHCI specification, Isochronous Receive DMA context has several
modes. One of mode is 'BufferFill' and Linux FireWire stack uses it to
receive isochronous packets for multiple isochronous channel as
FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL.
The mode is not used by in-kernel driver, while it's available for
userspace. The character device driver in firewire-core includes
cast of function callback for the mode since the type of callback
function is different from the other modes. The case is inconvenient
to effort of Control Flow Integrity builds due to
-Wcast-function-type warning.
This commit removes the cast. A inline helper function is newly added
to initialize isochronous context for the mode. The helper function
arranges isochronous context to assign specific callback function
after call of existent kernel API. It's noticeable that the number of
isochronous channel, speed, the size of header are not required for the
mode. The helper function is used for the mode by character device
driver instead of direct call of existent kernel API.
Changes in v2:
- unexport helper function
- use inline for helper function
- arrange arguments for helper function
- tested by libhinoko
Reported-by: Oscar Carter
Reference:
https://lore.kernel.org/lkml/20200519173425.4724-1-oscar.car...@gmx.com/
Signed-off-by: Takashi Sakamoto
---
drivers/firewire/core-cdev.c | 40 +++-
include/linux/firewire.h | 16 +++
2 files changed, 33 insertions(+), 23 deletions(-)
diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 6e291d8f3a27..7cbf6df34b43 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -957,7 +957,6 @@ static int ioctl_create_iso_context(struct client *client,
union ioctl_arg *arg)
{
struct fw_cdev_create_iso_context *a = >create_iso_context;
struct fw_iso_context *context;
- fw_iso_callback_t cb;
int ret;
BUILD_BUG_ON(FW_CDEV_ISO_CONTEXT_TRANSMIT != FW_ISO_CONTEXT_TRANSMIT ||
@@ -965,32 +964,27 @@ static int ioctl_create_iso_context(struct client
*client, union ioctl_arg *arg)
FW_CDEV_ISO_CONTEXT_RECEIVE_MULTICHANNEL !=
FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL);
- switch (a->type) {
- case FW_ISO_CONTEXT_TRANSMIT:
- if (a->speed > SCODE_3200 || a->channel > 63)
- return -EINVAL;
-
- cb = iso_callback;
- break;
-
- case FW_ISO_CONTEXT_RECEIVE:
- if (a->header_size < 4 || (a->header_size & 3) ||
- a->channel > 63)
- return -EINVAL;
-
- cb = iso_callback;
- break;
-
- case FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL:
- cb = (fw_iso_callback_t)iso_mc_callback;
- break;
+ if (a->type == FW_ISO_CONTEXT_TRANSMIT ||
+ a->type == FW_ISO_CONTEXT_RECEIVE) {
+ if (a->type == FW_ISO_CONTEXT_TRANSMIT) {
+ if (a->speed > SCODE_3200 || a->channel > 63)
+ return -EINVAL;
+ } else {
+ if (a->header_size < 4 || (a->header_size & 3) ||
+ a->channel > 63)
+ return -EINVAL;
+ }
- default:
+ context = fw_iso_context_create(client->device->card, a->type,
+ a->channel, a->speed, a->header_size,
+ iso_callback, client);
+ } else if (a->type == FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL) {
+ context = fw_iso_mc_context_create(client->device->card,
+ iso_mc_callback, client);
+ } else {
return -EINVAL;
}
- context = fw_iso_context_create(client->device->card, a->type,
- a->channel, a->speed, a->header_size, cb, client);
if (IS_ERR(context))
return PTR_ERR(context);
if (client->version < FW_CDEV_VERSION_AUTO_FLUSH_ISO_OVERFLOW)
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index aec8f30ab200..bff08118baaf 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -453,6 +453,22 @@ struct fw_iso_context {
struct fw_iso_context *fw_iso_context_create(struct fw_card *card,
int type, int channel, int speed, size_t header_size,
fw_iso_callback_t callback, void *callback_data);
+
+static inline struct fw_iso_context *fw_iso_mc_context_create(
+ struct fw_card *card,
+ fw_iso_mc_callback_t callback,
+
Re: [PATCH v2] firewire: Remove function callback casts
Hi Greg, On Sat, May 23, 2020 at 08:10:33AM +0200, Greg KH wrote: > On Fri, May 22, 2020 at 07:43:08PM +0200, Oscar Carter wrote: > > Hi, > > > > On Wed, May 20, 2020 at 03:16:24PM +0900, Takashi Sakamoto wrote: > > > Hi, > > > > > > I'm an author of ALSA firewire stack and thanks for the patch. I agree > > > with > > > your intention to remove the cast of function callback toward CFI build. > > > > > > Practically, the isochronous context with > > > FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL > > > is never used by in-kernel drivers. Here, I propose to leave current > > > kernel API (fw_iso_context_create() with fw_iso_callback_t) as is. > > If it's not used by anyone, why is it still there? Can't we just delete > it? For this patchset, I followed to the theory to keep backward compatibility when adding any change, and it's what I'd like to discuss. The isoc context of multichannel mode is also available for userspace applications, and libhinoko[1] uses it. In a point of backward compatibility for userspace, we can't delete the mode. (Practically, the mode is useful for the purpose of packet sniffing in bus and helpful to my work for development of ALSA firewire stack[2].) On the other hand, there's no unit driver to use the mode in upstream kernel. It's unlikely to use the mode for unit driver since the mode is not specific to unit functionality. In my opinion, it's reasonable to lose backward compatibility slightly by hiding the multichannel mode from in-kernel unit driver. I'll post v2 patchset later and I'd like you to merge them if no objections from the others for the loss of backward compatibility. [1] https://github.com/takaswie/libhinoko [2] https://mailman.alsa-project.org/pipermail/alsa-devel/2019-April/147862.html Regards Takashi Sakamoto
[PATCH 2/2] firewire-core: obsolete cast of function callback
This commit obsoletes cast of function callback to assist attempt of
Control Flow Integrity builds.
Reported-by: Oscar Carter
Reference:
https://lore.kernel.org/lkml/20200519173425.4724-1-oscar.car...@gmx.com/
Signed-off-by: Takashi Sakamoto
---
drivers/firewire/core-cdev.c | 44 +++-
1 file changed, 23 insertions(+), 21 deletions(-)
diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 6e291d8f3a27..f1e83396dd22 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -957,7 +957,6 @@ static int ioctl_create_iso_context(struct client *client,
union ioctl_arg *arg)
{
struct fw_cdev_create_iso_context *a = >create_iso_context;
struct fw_iso_context *context;
- fw_iso_callback_t cb;
int ret;
BUILD_BUG_ON(FW_CDEV_ISO_CONTEXT_TRANSMIT != FW_ISO_CONTEXT_TRANSMIT ||
@@ -965,32 +964,35 @@ static int ioctl_create_iso_context(struct client
*client, union ioctl_arg *arg)
FW_CDEV_ISO_CONTEXT_RECEIVE_MULTICHANNEL !=
FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL);
- switch (a->type) {
- case FW_ISO_CONTEXT_TRANSMIT:
- if (a->speed > SCODE_3200 || a->channel > 63)
- return -EINVAL;
-
- cb = iso_callback;
- break;
+ if (a->type != FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL) {
+ fw_iso_callback_t cb;
- case FW_ISO_CONTEXT_RECEIVE:
- if (a->header_size < 4 || (a->header_size & 3) ||
- a->channel > 63)
- return -EINVAL;
+ switch (a->type) {
+ case FW_ISO_CONTEXT_TRANSMIT:
+ if (a->speed > SCODE_3200 || a->channel > 63)
+ return -EINVAL;
- cb = iso_callback;
- break;
+ cb = iso_callback;
+ break;
- case FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL:
- cb = (fw_iso_callback_t)iso_mc_callback;
- break;
+ case FW_ISO_CONTEXT_RECEIVE:
+ if (a->header_size < 4 || (a->header_size & 3) ||
+ a->channel > 63)
+ return -EINVAL;
- default:
- return -EINVAL;
- }
+ cb = iso_callback;
+ break;
+ default:
+ return -EINVAL;
+ }
- context = fw_iso_context_create(client->device->card, a->type,
+ context = fw_iso_context_create(client->device->card, a->type,
a->channel, a->speed, a->header_size, cb, client);
+ } else {
+ context = fw_iso_mc_context_create(client->device->card,
+ a->type, a->channel, a->speed, a->header_size,
+ iso_mc_callback, client);
+ }
if (IS_ERR(context))
return PTR_ERR(context);
if (client->version < FW_CDEV_VERSION_AUTO_FLUSH_ISO_OVERFLOW)
--
2.25.1
[PATCH 0/2] firewire: obsolete cast of function callback toward CFI
Hi, Oscar Carter works for Control Flow Integrity build. Any cast of function callback is inconvenient for the work. Unfortunately, current code of firewire-core driver includes the cast[1] and Oscar posted some patches to remove it[2]. The patch is itself good. However, it includes changes existent kernel API and all of drivers as user of the API get affects from the change. This patchset is an alternative idea to add a new kernel API specific for multichannel isoc context. The existent kernel API and drivers is left as is. Practically, no in-kernel drivers use the additional API. Although the API is exported in the patchset, it's better to discuss about unexporting the API. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firewire/core-cdev.c#n985 [2] https://lore.kernel.org/lkml/20200519173425.4724-1-oscar.car...@gmx.com/ Regards Takashi Sakamoto (2): firewire-core: add kernel API to construct multichannel isoc context firewire-core: obsolete cast of function callback drivers/firewire/core-cdev.c | 44 +++- drivers/firewire/core-iso.c | 17 ++ include/linux/firewire.h | 3 +++ 3 files changed, 43 insertions(+), 21 deletions(-) -- 2.25.1
[PATCH 1/2] firewire-core: add kernel API to construct multichannel isoc context
In 1394 OHCI specification, IR context has several modes. One of mode
is 'multiChanMode'. For this mode, Linux FireWire stack has
FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL flag apart from FW_ISO_CONTEXT_RECEIVE,
and associated internal callback. However, code of firewire-core driver
includes cast of function callback for the mode and this brings
inconvenient to effort of Control Flow Integrity builds.
This commit is a preparation to remove the cast. A new kernel API for the
mode is added and existent API is specific for FW_ISO_CONTEXT_RECEIVE and
FW_ISO_CONTEXT_TRANSMIT modes. Actually, no in-kernel driver uses the mode
and the additional kernel API is never used at present.
Reported-by: Oscar Carter
Reference:
https://lore.kernel.org/lkml/20200519173425.4724-1-oscar.car...@gmx.com/
Signed-off-by: Takashi Sakamoto
---
drivers/firewire/core-iso.c | 17 +
include/linux/firewire.h| 3 +++
2 files changed, 20 insertions(+)
diff --git a/drivers/firewire/core-iso.c b/drivers/firewire/core-iso.c
index 185b0b78b3d6..07e967594f27 100644
--- a/drivers/firewire/core-iso.c
+++ b/drivers/firewire/core-iso.c
@@ -152,6 +152,23 @@ struct fw_iso_context *fw_iso_context_create(struct
fw_card *card,
}
EXPORT_SYMBOL(fw_iso_context_create);
+struct fw_iso_context *fw_iso_mc_context_create(struct fw_card *card,
+ int type, int channel, int speed, size_t header_size,
+ fw_iso_mc_callback_t callback, void *callback_data)
+{
+ struct fw_iso_context *ctx;
+
+ ctx = fw_iso_context_create(card, type, channel, speed, header_size,
+ NULL, callback_data);
+ if (IS_ERR(ctx))
+ return ctx;
+
+ ctx->callback.mc = callback;
+
+ return ctx;
+}
+EXPORT_SYMBOL(fw_iso_mc_context_create);
+
void fw_iso_context_destroy(struct fw_iso_context *ctx)
{
ctx->card->driver->free_iso_context(ctx);
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index aec8f30ab200..9477814ab12a 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -453,6 +453,9 @@ struct fw_iso_context {
struct fw_iso_context *fw_iso_context_create(struct fw_card *card,
int type, int channel, int speed, size_t header_size,
fw_iso_callback_t callback, void *callback_data);
+struct fw_iso_context *fw_iso_mc_context_create(struct fw_card *card,
+ int type, int channel, int speed, size_t header_size,
+ fw_iso_mc_callback_t callback, void *callback_data);
int fw_iso_context_set_channels(struct fw_iso_context *ctx, u64 *channels);
int fw_iso_context_queue(struct fw_iso_context *ctx,
struct fw_iso_packet *packet,
--
2.25.1
Re: [PATCH v2] firewire: Remove function callback casts
Hi, On Tue, May 19, 2020 at 07:34:25PM +0200, Oscar Carter wrote: > In an effort to enable -Wcast-function-type in the top-level Makefile to > support Control Flow Integrity builds, remove all the function callback > casts. > > To do this, modify the "fw_iso_context_create" function prototype adding > a new parameter for the multichannel callback. Also, fix all the > function calls accordingly. > > In the "fw_iso_context_create" function return an error code if both > callback parameters are NULL and also set the "ctx->callback.sc" > explicity to NULL in this case. It is not necessary set to NULL the > "ctx->callback.mc" variable because this and "ctx->callback.sc" are an > union and setting one implies setting the other one to the same value. > > Signed-off-by: Oscar Carter > --- > Changelog v1->v2 > -Set explicity to NULL the "ctx->callback.sc" variable and return an error > code in "fw_iso_context_create" function if both callback parameters are > NULL as Lev R. Oshvang suggested. > -Modify the commit changelog accordingly. > > drivers/firewire/core-cdev.c| 12 +++- > drivers/firewire/core-iso.c | 14 -- > drivers/firewire/net.c | 2 +- > drivers/media/firewire/firedtv-fw.c | 3 ++- > include/linux/firewire.h| 3 ++- > sound/firewire/amdtp-stream.c | 2 +- > sound/firewire/isight.c | 4 ++-- > 7 files changed, 27 insertions(+), 13 deletions(-) I'm an author of ALSA firewire stack and thanks for the patch. I agree with your intention to remove the cast of function callback toward CFI build. Practically, the isochronous context with FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL is never used by in-kernel drivers. Here, I propose to leave current kernel API (fw_iso_context_create() with fw_iso_callback_t) as is. Alternatively, a new kernel API for the context (e.g. fw_iso_mc_context_create() with fw_iso_mc_callback_t). This idea leaves current drivers as is and the change is done inner firewire-core driver, therefore existent kernel API is not changed. Later I post two patches for the proposal. I'd like you to review it and I'm glad to receive your comments. Regards Takashi Sakamoto
Re: [PATCH] ALSA: fireworks: Replace zero-length array with flexible-array
On Thu, May 07, 2020 at 01:52:45PM -0500, Gustavo A. R. Silva wrote:
> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array member[1][2],
> introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on.
>
> Also, notice that, dynamic memory allocations won't be affected by
> this change:
>
> "Flexible array members have incomplete type, and so the sizeof operator
> may not be applied. As a quirk of the original implementation of
> zero-length arrays, sizeof evaluates to zero."[1]
>
> sizeof(flexible-array-member) triggers a warning because flexible array
> members have incomplete type[1]. There are some instances of code in
> which the sizeof operator is being incorrectly/erroneously applied to
> zero-length arrays and the result is zero. Such instances may be hiding
> some bugs. So, this work (flexible-array member conversions) will also
> help to get completely rid of those sorts of issues.
>
> This issue was found with the help of Coccinelle.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva
> ---
> sound/firewire/fireworks/fireworks.h |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Takashi Sakamoto
> diff --git a/sound/firewire/fireworks/fireworks.h
> b/sound/firewire/fireworks/fireworks.h
> index dda797209a27..654e28a6669f 100644
> --- a/sound/firewire/fireworks/fireworks.h
> +++ b/sound/firewire/fireworks/fireworks.h
> @@ -177,7 +177,7 @@ struct snd_efw_phys_meters {
> u32 in_meters;
> u32 reserved4;
> u32 reserved5;
> - u32 values[0];
> + u32 values[];
> } __packed;
> enum snd_efw_clock_source {
> SND_EFW_CLOCK_SOURCE_INTERNAL = 0,
Thanks
Takashi Sakamoto
[PATCH] MAINTAINERS: update entry for firewire audio drivers with UAPI header
The most of drivers in ALSA firewire stack supports common ioctl commands to enable/disable packet streaming as well as some ioctl commands for model-specific features. An UAPI header is exported to userspace. This commit adds supplement for entry of ALSA firewire stack with a path of the UAPI header. Signed-off-by: Takashi Sakamoto --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 4200194e69ea..6bd54ce6dc66 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6271,6 +6271,7 @@ L:alsa-de...@alsa-project.org (moderated for non-subscribers) T: git git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git S: Maintained F: sound/firewire/ +F: include/uapi/sound/firewire.h FIREWIRE MEDIA DRIVERS (firedtv) M: Stefan Richter -- 2.20.1
Re: [PATCH] MAINTAINERS: update entry of firewire audio drivers
On Sat, Aug 31, 2019 at 05:27:13PM +0200, Takashi Iwai wrote: > On Fri, 30 Aug 2019 15:24:46 +0200, > Takashi Sakamoto wrote: > > > > This commit adds myself as one of maintainers for firewire audio > > drivers and IEC 61883-1/6 packet streaming engine. I call them ALSA > > firewire stack as a whole. > > > > 6 years ago I joined in development for this category of drivers with > > heavy reverse-engineering tasks and over 100 models are now available > > from ALSA applications. IEEE 1394 bus itself and units on the bus are > > enough legacy but the development still continues. > > > > I have a plan to add drastic enhancement in kernel v5.5 and v5.6 period. > > This commit adds myself into MAINTAINERS so that developers and users > > can easily find active developer to post their issues, especially for > > regression. > > > > Signed-off-by: Takashi Sakamoto > > Applied now (with a typo correction -- a superfluous '+' was added). > Thanks. > > > Takashi Oops, thanks for your care to my typo... > > --- > > MAINTAINERS | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 24e29b2e53c9..8929a2ec75f7 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -6264,8 +6264,9 @@ S:Maintained > > F: drivers/hwmon/f75375s.c > > F: include/linux/f75375s.h > > > > -FIREWIRE AUDIO DRIVERS > > +FIREWIRE AUDIO DRIVERS and IEC 61883-1/6 PACKET STREAMING ENGINE > > M: Clemens Ladisch > > ++M: Takashi Sakamoto > > L: alsa-de...@alsa-project.org (moderated for non-subscribers) > > T: git git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git > > S: Maintained > > -- > > 2.20.1 > > Regards Takashi Sakamoto
[PATCH] MAINTAINERS: update entry of firewire audio drivers
This commit adds myself as one of maintainers for firewire audio drivers and IEC 61883-1/6 packet streaming engine. I call them ALSA firewire stack as a whole. 6 years ago I joined in development for this category of drivers with heavy reverse-engineering tasks and over 100 models are now available from ALSA applications. IEEE 1394 bus itself and units on the bus are enough legacy but the development still continues. I have a plan to add drastic enhancement in kernel v5.5 and v5.6 period. This commit adds myself into MAINTAINERS so that developers and users can easily find active developer to post their issues, especially for regression. Signed-off-by: Takashi Sakamoto --- MAINTAINERS | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 24e29b2e53c9..8929a2ec75f7 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6264,8 +6264,9 @@ S:Maintained F: drivers/hwmon/f75375s.c F: include/linux/f75375s.h -FIREWIRE AUDIO DRIVERS +FIREWIRE AUDIO DRIVERS and IEC 61883-1/6 PACKET STREAMING ENGINE M: Clemens Ladisch ++M:Takashi Sakamoto L: alsa-de...@alsa-project.org (moderated for non-subscribers) T: git git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git S: Maintained -- 2.20.1
Re: [alsa-devel] [PATCH] ALSA: firewire: fix a memory leak bug
Hi,
On Thu, Aug 8, 2019, at 14:53, Wenwen Wang wrote:
> In iso_packets_buffer_init(), 'b->packets' is allocated through
> kmalloc_array(). Then, the aligned packet size is checked. If it is
> larger than PAGE_SIZE, -EINVAL will be returned to indicate the error.
> However, the allocated 'b->packets' is not deallocated on this path,
> leading to a memory leak.
>
> To fix the above issue, free 'b->packets' before returning the error code.
>
> Signed-off-by: Wenwen Wang
> ---
> sound/firewire/packets-buffer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Takashi Sakamoto
And this bug exists till its first commit for v2.6.39.
Fixes: 31ef9134eb52 ("ALSA: add LaCie FireWire Speakers/Griffin FireWave
Surround driver")
Cc: # v2.6.39+
Thanks
Takashi Sakamoto
Re: [alsa-devel] [PATCH][next] ALSA: firewire-lib: remove redundant assignment to cip_header
Hi Colin,
On Sat, May 25, 2019, at 06:35, Colin King wrote:
> From: Colin Ian King
>
> The assignement to cip_header is redundant as the value never
> read and it is being re-assigned in the if and else paths of
> the following if statement. Clean up the code by removing it.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King
> ---
> sound/firewire/amdtp-stream.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/sound/firewire/amdtp-stream.c
> b/sound/firewire/amdtp-stream.c
> index 2d9c764061d1..4236955bbf57 100644
> --- a/sound/firewire/amdtp-stream.c
> +++ b/sound/firewire/amdtp-stream.c
> @@ -675,7 +675,6 @@ static int handle_in_packet(struct amdtp_stream *s,
> unsigned int cycle,
> return -EIO;
> }
>
> - cip_header = ctx_header + 2;
> if (!(s->flags & CIP_NO_HEADER)) {
> cip_header = _header[2];
> err = check_cip_header(s, cip_header, payload_length,
Thanks for the fix. I've already posted further patch for refactoring
and this was also fixed by a commit 98e3e43b599d ("
ALSA: firewire-lib: refactoring to obsolete IR packet handler").
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?id=98e3e43b599d742c104864c6772a251025ffb52b
Thanks
Takashi Sakamoto
Re: [PATCH] MAINTAINERS: update git tree for sound entries
Hi, On Thu, May 02, 2019 at 11:27:00AM -0600, Ross Zwisler wrote: > Several sound related entries in MAINTAINERS refer to the old git tree > at "git://git.alsa-project.org/alsa-kernel.git". This is no longer used > for development, and Takashi Iwai's kernel.org tree is used instead. > > Signed-off-by: Ross Zwisler > --- > MAINTAINERS | 13 ++--- > 1 file changed, 6 insertions(+), 7 deletions(-) This is a good catch. Reviewed-by: Takashi Sakamoto > diff --git a/MAINTAINERS b/MAINTAINERS > index e17ebf70b5480..d373d976a9317 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3351,7 +3351,7 @@ F: include/uapi/linux/bsg.h > BT87X AUDIO DRIVER > M: Clemens Ladisch > L: alsa-de...@alsa-project.org (moderated for non-subscribers) > -T: git git://git.alsa-project.org/alsa-kernel.git > +T: git git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git > S: Maintained > F: Documentation/sound/cards/bt87x.rst > F: sound/pci/bt87x.c > @@ -3404,7 +3404,7 @@ F: drivers/scsi/FlashPoint.* > C-MEDIA CMI8788 DRIVER > M: Clemens Ladisch > L: alsa-de...@alsa-project.org (moderated for non-subscribers) > -T: git git://git.alsa-project.org/alsa-kernel.git > +T: git git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git > S: Maintained > F: sound/pci/oxygen/ > > @@ -5696,7 +5696,7 @@ F: drivers/edac/qcom_edac.c > EDIROL UA-101/UA-1000 DRIVER > M: Clemens Ladisch > L: alsa-de...@alsa-project.org (moderated for non-subscribers) > -T: git git://git.alsa-project.org/alsa-kernel.git > +T: git git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git > S: Maintained > F: sound/usb/misc/ua101.c > > @@ -6036,7 +6036,7 @@ F: include/linux/f75375s.h > FIREWIRE AUDIO DRIVERS > M: Clemens Ladisch > L: alsa-de...@alsa-project.org (moderated for non-subscribers) > -T: git git://git.alsa-project.org/alsa-kernel.git > +T: git git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git > S: Maintained > F: sound/firewire/ > > @@ -11593,7 +11593,7 @@ F:Documentation/devicetree/bindings/opp/ > OPL4 DRIVER > M: Clemens Ladisch > L: alsa-de...@alsa-project.org (moderated for non-subscribers) > -T: git git://git.alsa-project.org/alsa-kernel.git > +T: git git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git > S: Maintained > F: sound/drivers/opl4/ > > @@ -14490,7 +14490,6 @@ M:Takashi Iwai > L: alsa-de...@alsa-project.org (moderated for non-subscribers) > W: http://www.alsa-project.org/ > T: git git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git > -T: git git://git.alsa-project.org/alsa-kernel.git > Q: http://patchwork.kernel.org/project/alsa-devel/list/ > S: Maintained > F: Documentation/sound/ > @@ -16100,7 +16099,7 @@ F:drivers/usb/storage/ > USB MIDI DRIVER > M: Clemens Ladisch > L: alsa-de...@alsa-project.org (moderated for non-subscribers) > -T: git git://git.alsa-project.org/alsa-kernel.git > +T: git git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git > S: Maintained > F: sound/usb/midi.* > > -- > 2.21.0.593.g511ec345e18-goog Regards Takashi Sakamoto
Re: [PATCH 17/28] tools include uapi: Update asound.h copy
Hi, On 2018/11/01 22:04, Arnaldo Carvalho de Melo wrote: Em Thu, Nov 01, 2018 at 08:54:45PM +0900, Takashi Sakamoto escreveu: Hi Arnaldo, On Nov 1 2018 4:29, Arnaldo Carvalho de Melo wrote: Wouldn't a symlink be simpler? That would be equivalent to using it directly, see my response to Takashi for a few of the reasons this is done this way. I have a plan to add changes to 'include/uapi/sound/asound.h' in a development period for Linux kernel v4.21[1]. Would I need to add you to C.C list of the future patch to notify the change, or things go well without such special care for your side? Don't worry, the system was designed not to create any extra work for kernel developers. The CC messages is just informative. :-) - Arnaldo [1] https://github.com/takaswie/presentations/blob/master/20181021/contents.md Thanks, and I note that my work will neither change value of each ioctl command for PCM/Control interfaces nor add new commands, thus no worries in your side at this point. I'll enjoy the beauty output from perf tools after rebasing my runtime environment to v4.19 kernel, Thank you. Takashi Sakamoto
Re: [PATCH 17/28] tools include uapi: Update asound.h copy
Hi, On 2018/11/01 22:04, Arnaldo Carvalho de Melo wrote: Em Thu, Nov 01, 2018 at 08:54:45PM +0900, Takashi Sakamoto escreveu: Hi Arnaldo, On Nov 1 2018 4:29, Arnaldo Carvalho de Melo wrote: Wouldn't a symlink be simpler? That would be equivalent to using it directly, see my response to Takashi for a few of the reasons this is done this way. I have a plan to add changes to 'include/uapi/sound/asound.h' in a development period for Linux kernel v4.21[1]. Would I need to add you to C.C list of the future patch to notify the change, or things go well without such special care for your side? Don't worry, the system was designed not to create any extra work for kernel developers. The CC messages is just informative. :-) - Arnaldo [1] https://github.com/takaswie/presentations/blob/master/20181021/contents.md Thanks, and I note that my work will neither change value of each ioctl command for PCM/Control interfaces nor add new commands, thus no worries in your side at this point. I'll enjoy the beauty output from perf tools after rebasing my runtime environment to v4.19 kernel, Thank you. Takashi Sakamoto
Re: [PATCH 17/28] tools include uapi: Update asound.h copy
Hi Arnaldo, On Nov 1 2018 4:29, Arnaldo Carvalho de Melo wrote: Wouldn't a symlink be simpler? That would be equivalent to using it directly, see my response to Takashi for a few of the reasons this is done this way. I have a plan to add changes to 'include/uapi/sound/asound.h' in a development period for Linux kernel v4.21[1]. Would I need to add you to C.C list of the future patch to notify the change, or things go well without such special care for your side? [1] https://github.com/takaswie/presentations/blob/master/20181021/contents.md Regards Takashi Sakamoto
Re: [PATCH 17/28] tools include uapi: Update asound.h copy
Hi Arnaldo, On Nov 1 2018 4:29, Arnaldo Carvalho de Melo wrote: Wouldn't a symlink be simpler? That would be equivalent to using it directly, see my response to Takashi for a few of the reasons this is done this way. I have a plan to add changes to 'include/uapi/sound/asound.h' in a development period for Linux kernel v4.21[1]. Would I need to add you to C.C list of the future patch to notify the change, or things go well without such special care for your side? [1] https://github.com/takaswie/presentations/blob/master/20181021/contents.md Regards Takashi Sakamoto
Re: [PATCH 0/4] Various cleanup + Mic Fix
On 2018年10月09日 04:39, Connor McAdams wrote: This patch set fixes the microphone inconsistency issue, which means the microphone now works all the time on all of the cards I've tested (ZxR, Z, AE-5), along with the input effects. It also includes changes suggested by Takashi Sakamoto, I believe I did what he asked properly, but if I messed it up I'm sure you guys will let me know. This should finish up most of the ca0132 work, with all inputs and outputs working on the desktop cards. Connor McAdams (4): ALSA: hda/ca0132 - Fix microphone inconsistency issues ALSA: hda/ca0132 - Clean up patch_ca0132() ALSA: hda/ca0132 - Add error checking in ca0132_build_controls() ALSA: hda/ca0132 - Fix input effect controls for desktop cards sound/pci/hda/patch_ca0132.c | 75 +++- 1 file changed, 54 insertions(+), 21 deletions(-) Reviewed-by: Takashi Sakamoto Thanks Takashi Sakamoto
Re: [PATCH 0/4] Various cleanup + Mic Fix
On 2018年10月09日 04:39, Connor McAdams wrote: This patch set fixes the microphone inconsistency issue, which means the microphone now works all the time on all of the cards I've tested (ZxR, Z, AE-5), along with the input effects. It also includes changes suggested by Takashi Sakamoto, I believe I did what he asked properly, but if I messed it up I'm sure you guys will let me know. This should finish up most of the ca0132 work, with all inputs and outputs working on the desktop cards. Connor McAdams (4): ALSA: hda/ca0132 - Fix microphone inconsistency issues ALSA: hda/ca0132 - Clean up patch_ca0132() ALSA: hda/ca0132 - Add error checking in ca0132_build_controls() ALSA: hda/ca0132 - Fix input effect controls for desktop cards sound/pci/hda/patch_ca0132.c | 75 +++- 1 file changed, 54 insertions(+), 21 deletions(-) Reviewed-by: Takashi Sakamoto Thanks Takashi Sakamoto
Re: [PATCH 00/11] Add ZxR support + bugfixes
Hi, On Sep 30 2018 12:03, Connor McAdams wrote: This patch series adds support for the Sound Blaster ZxR, as well as a few bug fixes. This should be the last ca0132 based Creative card that needed support to be added. Also, I did check to make sure each patch compiles properly this time, but you can check yourself just to be sure. :) Connor McAdams (11): ALSA: hda/ca0132 - Fix AE-5 control type ALSA: hda/ca0132 - Fix surround sound with output effects ALSA: hda/ca0132 - Add ZxR quirks + new quirk check function ALSA: hda/ca0132 - Add ZxR pincfg ALSA: hda/ca0132 - Add DBpro hda_codec_ops ALSA: hda/ca0132 - Add ZxR init commands ALSA: hda/ca0132 - Add ZxR DSP post-download commands ALSA: hda/ca0132 - Add ZxR input/output select commands ALSA: hda/ca0132 - Remove input select enum for ZxR ALSA: hda/ca0132 - Add ZxR 600 ohm gain control ALSA: hda/ca0132 - Add ZxR exit commands sound/pci/hda/patch_ca0132.c | 369 --- 1 file changed, 345 insertions(+), 24 deletions(-) I reviewed the above patches and they looks good to be merged to upstream. Reviewed-by: Takashi Sakamoto I've already point some nitpickings but they can be solved after merging them till next merge window. Thanks Takashi Sakamoto
Re: [PATCH 00/11] Add ZxR support + bugfixes
Hi, On Sep 30 2018 12:03, Connor McAdams wrote: This patch series adds support for the Sound Blaster ZxR, as well as a few bug fixes. This should be the last ca0132 based Creative card that needed support to be added. Also, I did check to make sure each patch compiles properly this time, but you can check yourself just to be sure. :) Connor McAdams (11): ALSA: hda/ca0132 - Fix AE-5 control type ALSA: hda/ca0132 - Fix surround sound with output effects ALSA: hda/ca0132 - Add ZxR quirks + new quirk check function ALSA: hda/ca0132 - Add ZxR pincfg ALSA: hda/ca0132 - Add DBpro hda_codec_ops ALSA: hda/ca0132 - Add ZxR init commands ALSA: hda/ca0132 - Add ZxR DSP post-download commands ALSA: hda/ca0132 - Add ZxR input/output select commands ALSA: hda/ca0132 - Remove input select enum for ZxR ALSA: hda/ca0132 - Add ZxR 600 ohm gain control ALSA: hda/ca0132 - Add ZxR exit commands sound/pci/hda/patch_ca0132.c | 369 --- 1 file changed, 345 insertions(+), 24 deletions(-) I reviewed the above patches and they looks good to be merged to upstream. Reviewed-by: Takashi Sakamoto I've already point some nitpickings but they can be solved after merging them till next merge window. Thanks Takashi Sakamoto
Re: [PATCH 10/11] ALSA: hda/ca0132 - Add ZxR 600 ohm gain control
Hi, On Sep 30 2018 12:03, Connor McAdams wrote: This patch adds a control for 600 ohm gain on the Sound Blaster ZxR. Signed-off-by: Connor McAdams --- sound/pci/hda/patch_ca0132.c | 44 ++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c index f0781e4..90e6a96 100644 --- a/sound/pci/hda/patch_ca0132.c +++ b/sound/pci/hda/patch_ca0132.c .. @@ -6415,6 +6452,9 @@ static int ca0132_build_controls(struct hda_codec *codec) ae5_add_headphone_gain_enum(codec); ae5_add_sound_filter_enum(codec); } + + if (spec->quirk == QUIRK_ZXR) + zxr_add_headphone_gain_switch(codec); #ifdef ENABLE_TUNING_CONTROLS add_tuning_ctls(codec); #endif Though error code can be returned in a call of snd_hda_ctl_add(), it's not handled correctly in 'ca0132_build_controls()'. At least, return code in calls of below functions is better to be checked. - add_voicefx() - add_ca0132_alt_eq_presets() - ca0132_alt_add_svm_enum() - ca0132_alt_add_output_enum - ca0132_alt_add_input_enum - ca0132_alt_add_mic_boost_enum - ae5_add_headphone_gain_enum - ae5_add_sound_filter_enum - zxr_add_headphone_gain_switch This is not a strong request and you can work for it after merging this patchset. We have two weeks more till next merge window. Regards Takashi Sakamoto
Re: [PATCH 10/11] ALSA: hda/ca0132 - Add ZxR 600 ohm gain control
Hi, On Sep 30 2018 12:03, Connor McAdams wrote: This patch adds a control for 600 ohm gain on the Sound Blaster ZxR. Signed-off-by: Connor McAdams --- sound/pci/hda/patch_ca0132.c | 44 ++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c index f0781e4..90e6a96 100644 --- a/sound/pci/hda/patch_ca0132.c +++ b/sound/pci/hda/patch_ca0132.c .. @@ -6415,6 +6452,9 @@ static int ca0132_build_controls(struct hda_codec *codec) ae5_add_headphone_gain_enum(codec); ae5_add_sound_filter_enum(codec); } + + if (spec->quirk == QUIRK_ZXR) + zxr_add_headphone_gain_switch(codec); #ifdef ENABLE_TUNING_CONTROLS add_tuning_ctls(codec); #endif Though error code can be returned in a call of snd_hda_ctl_add(), it's not handled correctly in 'ca0132_build_controls()'. At least, return code in calls of below functions is better to be checked. - add_voicefx() - add_ca0132_alt_eq_presets() - ca0132_alt_add_svm_enum() - ca0132_alt_add_output_enum - ca0132_alt_add_input_enum - ca0132_alt_add_mic_boost_enum - ae5_add_headphone_gain_enum - ae5_add_sound_filter_enum - zxr_add_headphone_gain_switch This is not a strong request and you can work for it after merging this patchset. We have two weeks more till next merge window. Regards Takashi Sakamoto
Re: [PATCH 05/11] ALSA: hda/ca0132 - Add DBpro hda_codec_ops
Hi,
On Sep 30 2018 12:03, Connor McAdams wrote:
This patch adds separate hda_codec_ops for the DBPro daughter board, as
it behaves more like a generic HDA codec than the other ca0132 cards,
despite having a ca0132 on board.
Signed-off-by: Connor McAdams
---
sound/pci/hda/patch_ca0132.c | 100 +++
1 file changed, 100 insertions(+)
diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index 4d23eb9..a543b23 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
...
@@ -8192,6 +8282,13 @@ static const struct hda_codec_ops ca0132_patch_ops = {
...
+
static void ca0132_config(struct hda_codec *codec)
{
struct ca0132_spec *spec = codec->spec;
@@ -8488,6 +8585,9 @@ static int patch_ca0132(struct hda_codec *codec)
spec->mixers[0] = desktop_mixer;
snd_hda_codec_set_name(codec, "Sound Blaster Z");
break;
+ case QUIRK_ZXR_DBPRO:
+ codec->patch_ops = dbpro_patch_ops;
+ break;
This patch looks good to me, but in a view of prevention from
errors in future work (you will do), it's not better to assign to
'struct hda_codec.patch_ops' in different lines which have larger
distance.
8551 static int patch_ca0132(struct hda_codec *codec)
8552 {
...
8565 codec->patch_ops = ca0132_patch_ops;
...
8588 case QUIRK_ZXR_DBPRO:
8589 codec->patch_ops = dbpro_patch_ops;
...
This is not a strong request but I recommend you to reorder procedures
done in 'patch_ca0132()' so that:
patch_ca0132()
->kzalloc(sizeof(struct ca0132_spec))
->snd_pci_quirk_lookup() and sbz_detect_quirk()
->'codec' preparation (assignment to members in hda_codec, etc.)
->'spec' preparation (assignment to members in ca0132_spec, etc.)
Regards
Takashi Sakamoto
Re: [PATCH 05/11] ALSA: hda/ca0132 - Add DBpro hda_codec_ops
Hi,
On Sep 30 2018 12:03, Connor McAdams wrote:
This patch adds separate hda_codec_ops for the DBPro daughter board, as
it behaves more like a generic HDA codec than the other ca0132 cards,
despite having a ca0132 on board.
Signed-off-by: Connor McAdams
---
sound/pci/hda/patch_ca0132.c | 100 +++
1 file changed, 100 insertions(+)
diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index 4d23eb9..a543b23 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
...
@@ -8192,6 +8282,13 @@ static const struct hda_codec_ops ca0132_patch_ops = {
...
+
static void ca0132_config(struct hda_codec *codec)
{
struct ca0132_spec *spec = codec->spec;
@@ -8488,6 +8585,9 @@ static int patch_ca0132(struct hda_codec *codec)
spec->mixers[0] = desktop_mixer;
snd_hda_codec_set_name(codec, "Sound Blaster Z");
break;
+ case QUIRK_ZXR_DBPRO:
+ codec->patch_ops = dbpro_patch_ops;
+ break;
This patch looks good to me, but in a view of prevention from
errors in future work (you will do), it's not better to assign to
'struct hda_codec.patch_ops' in different lines which have larger
distance.
8551 static int patch_ca0132(struct hda_codec *codec)
8552 {
...
8565 codec->patch_ops = ca0132_patch_ops;
...
8588 case QUIRK_ZXR_DBPRO:
8589 codec->patch_ops = dbpro_patch_ops;
...
This is not a strong request but I recommend you to reorder procedures
done in 'patch_ca0132()' so that:
patch_ca0132()
->kzalloc(sizeof(struct ca0132_spec))
->snd_pci_quirk_lookup() and sbz_detect_quirk()
->'codec' preparation (assignment to members in hda_codec, etc.)
->'spec' preparation (assignment to members in ca0132_spec, etc.)
Regards
Takashi Sakamoto
Re: [PATCH] Documentation: soundwire: fix stream.rst markup warnings
Hi,
On Sep 17 2018 09:34, Randy Dunlap wrote:
From: Randy Dunlap
Fix kernel-doc markup warnings in soundwire/stream.rst:
rc4/Documentation/driver-api/soundwire/stream.rst:177: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
rc4/Documentation/driver-api/soundwire/stream.rst:203: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
rc4/Documentation/driver-api/soundwire/stream.rst:248: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
rc4/Documentation/driver-api/soundwire/stream.rst:277: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
rc4/Documentation/driver-api/soundwire/stream.rst:304: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
rc4/Documentation/driver-api/soundwire/stream.rst:328: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
rc4/Documentation/driver-api/soundwire/stream.rst:352: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
rc4/Documentation/driver-api/soundwire/stream.rst:364: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
Fixes: 89634f99a83e ("Documentation: soundwire: Add more documentation")
Signed-off-by: Randy Dunlap
Cc: Pierre-Louis Bossart
Cc: Sanyog Kale
Cc: Shreyas NC
Cc: Vinod Koul
Cc: alsa-de...@alsa-project.org
Cc: linux-...@vger.kernel.org
---
Documentation/driver-api/soundwire/stream.rst |8
1 file changed, 8 insertions(+)
Reviewed-by: Takashi Sakamoto
Tested-by: Takashi Sakamoto
Thanks
Takashi Sakamoto
Re: [PATCH] Documentation: soundwire: fix stream.rst markup warnings
Hi,
On Sep 17 2018 09:34, Randy Dunlap wrote:
From: Randy Dunlap
Fix kernel-doc markup warnings in soundwire/stream.rst:
rc4/Documentation/driver-api/soundwire/stream.rst:177: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
rc4/Documentation/driver-api/soundwire/stream.rst:203: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
rc4/Documentation/driver-api/soundwire/stream.rst:248: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
rc4/Documentation/driver-api/soundwire/stream.rst:277: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
rc4/Documentation/driver-api/soundwire/stream.rst:304: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
rc4/Documentation/driver-api/soundwire/stream.rst:328: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
rc4/Documentation/driver-api/soundwire/stream.rst:352: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
rc4/Documentation/driver-api/soundwire/stream.rst:364: WARNING: Explicit markup
ends without a blank line; unexpected unindent.
Fixes: 89634f99a83e ("Documentation: soundwire: Add more documentation")
Signed-off-by: Randy Dunlap
Cc: Pierre-Louis Bossart
Cc: Sanyog Kale
Cc: Shreyas NC
Cc: Vinod Koul
Cc: alsa-de...@alsa-project.org
Cc: linux-...@vger.kernel.org
---
Documentation/driver-api/soundwire/stream.rst |8
1 file changed, 8 insertions(+)
Reviewed-by: Takashi Sakamoto
Tested-by: Takashi Sakamoto
Thanks
Takashi Sakamoto
Re: [PATCH 08/11] UAPI: sound: Fix use of u32 and co. in UAPI headers
Hi, On Sep 6 2018 00:55, David Howells wrote: Fix the use of u32 and co. in UAPI headers as these are not defined. Switch to using the __u32-style equivalents instead. Signed-off-by: David Howells cc: Jaroslav Kysela cc: Takashi Iwai cc: alsa-de...@alsa-project.org (moderated for non-subscribers) --- include/uapi/sound/skl-tplg-interface.h | 106 --- 1 file changed, 54 insertions(+), 52 deletions(-) A similar patch was already proposed[1] and has been applied by Mark to his tree[2]. Your issue (3) is going to be solved soon for v4.19 kernel. [1] [alsa-devel] [PATCH] uapi: fix sound/skl-tplg-interface.h userspace compilation errors http://mailman.alsa-project.org/pipermail/alsa-devel/2018-August/139392.html [2] ASoC: uapi: fix sound/skl-tplg-interface.h userspace compilation errors https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/log/?h=for-4.19 Thanks Takashi Sakamoto
Re: [PATCH 08/11] UAPI: sound: Fix use of u32 and co. in UAPI headers
Hi, On Sep 6 2018 00:55, David Howells wrote: Fix the use of u32 and co. in UAPI headers as these are not defined. Switch to using the __u32-style equivalents instead. Signed-off-by: David Howells cc: Jaroslav Kysela cc: Takashi Iwai cc: alsa-de...@alsa-project.org (moderated for non-subscribers) --- include/uapi/sound/skl-tplg-interface.h | 106 --- 1 file changed, 54 insertions(+), 52 deletions(-) A similar patch was already proposed[1] and has been applied by Mark to his tree[2]. Your issue (3) is going to be solved soon for v4.19 kernel. [1] [alsa-devel] [PATCH] uapi: fix sound/skl-tplg-interface.h userspace compilation errors http://mailman.alsa-project.org/pipermail/alsa-devel/2018-August/139392.html [2] ASoC: uapi: fix sound/skl-tplg-interface.h userspace compilation errors https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/log/?h=for-4.19 Thanks Takashi Sakamoto
Re: [PATCH] ALSA: unhide snd_soc_new_compress() declaration
Hi,
On May 31 2018 07:06, Arnd Bergmann wrote:
My randconfig build setup came across a new build failure today:
sound/soc/soc-topology.c: In function 'soc_tplg_dai_create':
sound/soc/soc-topology.c:1758:27: error: 'snd_soc_new_compress' undeclared
(first use in this function); did you mean 'snd_soc_compr_ops'?
dai_drv->compress_new = snd_soc_new_compress;
Simply making that prototype visible again fixes the build failure
for me, though I'm not exactly sure why we don't get a link
error now.
The reference to snd_soc_new_compress() was added a few weeks
ago and seems to have caused the problem.
Fixes: 5db6aab6f36f ("ASoC: topology: Add support for compressed PCMs")
Signed-off-by: Arnd Bergmann
---
include/sound/soc.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h
index 600a7ebd10c0..8e073865bde3 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -460,9 +460,7 @@ struct snd_soc_component *snd_soc_lookup_component(struct
device *dev,
const char *driver_name);
int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num);
-#ifdef CONFIG_SND_SOC_COMPRESS
int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num);
-#endif
void snd_soc_disconnect_sync(struct device *dev);
This bug was fixed by a commit 0b014d72ebae ('ASoC: fix 0-day warnings
with snd_soc_new_compress()')[1][2] in Mark's tree. It applies an
different solution to add an alternative inline function.
A small nitpicking; please add 'ASoC' prefix when posting patches to
ALSA SoC part, instead of 'ALSA'.
[1] [alsa-devel] [PATCH] ASoC: fix 0-day warnings with
snd_soc_new_compress()
http://mailman.alsa-project.org/pipermail/alsa-devel/2018-May/136620.html
[2] ASoC: fix 0-day warnings with snd_soc_new_compress()
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=for-4.18=0b014d72ebae14c0c6ab3fb36a442fda91e1a1b3
Thanks
Takashi Sakamoto
Re: [PATCH] ALSA: unhide snd_soc_new_compress() declaration
Hi,
On May 31 2018 07:06, Arnd Bergmann wrote:
My randconfig build setup came across a new build failure today:
sound/soc/soc-topology.c: In function 'soc_tplg_dai_create':
sound/soc/soc-topology.c:1758:27: error: 'snd_soc_new_compress' undeclared
(first use in this function); did you mean 'snd_soc_compr_ops'?
dai_drv->compress_new = snd_soc_new_compress;
Simply making that prototype visible again fixes the build failure
for me, though I'm not exactly sure why we don't get a link
error now.
The reference to snd_soc_new_compress() was added a few weeks
ago and seems to have caused the problem.
Fixes: 5db6aab6f36f ("ASoC: topology: Add support for compressed PCMs")
Signed-off-by: Arnd Bergmann
---
include/sound/soc.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h
index 600a7ebd10c0..8e073865bde3 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -460,9 +460,7 @@ struct snd_soc_component *snd_soc_lookup_component(struct
device *dev,
const char *driver_name);
int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num);
-#ifdef CONFIG_SND_SOC_COMPRESS
int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num);
-#endif
void snd_soc_disconnect_sync(struct device *dev);
This bug was fixed by a commit 0b014d72ebae ('ASoC: fix 0-day warnings
with snd_soc_new_compress()')[1][2] in Mark's tree. It applies an
different solution to add an alternative inline function.
A small nitpicking; please add 'ASoC' prefix when posting patches to
ALSA SoC part, instead of 'ALSA'.
[1] [alsa-devel] [PATCH] ASoC: fix 0-day warnings with
snd_soc_new_compress()
http://mailman.alsa-project.org/pipermail/alsa-devel/2018-May/136620.html
[2] ASoC: fix 0-day warnings with snd_soc_new_compress()
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=for-4.18=0b014d72ebae14c0c6ab3fb36a442fda91e1a1b3
Thanks
Takashi Sakamoto
Re: [PATCH][next] ALSA: xen-front: fix unsigned error check on return from to_sndif_format
Hi,
On May 28 2018 06:32, Colin King wrote:
From: Colin Ian King <colin.k...@canonical.com>
The negative error return from the call to to_sndif_format is being
assigned to an unsigned 8 bit integer and hence the check for a negative
value is always going to be false. Fix this by using ret as the error
return and hence the negative error can be detected and assign
the u8 sndif_format to ret if there is no error.
Detected by CoverityScan, CID#1469385 ("Unsigned compared against 0")
Signed-off-by: Colin Ian King <colin.k...@canonical.com>
---
sound/xen/xen_snd_front_alsa.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/sound/xen/xen_snd_front_alsa.c b/sound/xen/xen_snd_front_alsa.c
index 5041f83e98d2..5a2bd70a2fa1 100644
--- a/sound/xen/xen_snd_front_alsa.c
+++ b/sound/xen/xen_snd_front_alsa.c
@@ -466,13 +466,14 @@ static int alsa_prepare(struct snd_pcm_substream
*substream)
u8 sndif_format;
int ret;
- sndif_format = to_sndif_format(runtime->format);
- if (sndif_format < 0) {
+ ret = to_sndif_format(runtime->format);
+ if (ret < 0) {
dev_err(>front_info->xb_dev->dev,
"Unsupported sample format: %d\n",
runtime->format);
- return sndif_format;
+ return ret;
}
+ sndif_format = ret;
ret = xen_snd_front_stream_prepare(>evt_pair->req,
>sh_buf,
Indeed. A typical assignment mistake. Instead, we could change the type
of 'sndif_format' to signed int, however in this case it's not the same
as the third argument of xen_snd_front_stream_prepare() because it is
'u8'. This patch looks good to me.
Reviewed-by: Takashi Sakamoto <o-taka...@sakamoccchi.jp>
Regards
Takashi Sakamoto
Re: [PATCH][next] ALSA: xen-front: fix unsigned error check on return from to_sndif_format
Hi,
On May 28 2018 06:32, Colin King wrote:
From: Colin Ian King
The negative error return from the call to to_sndif_format is being
assigned to an unsigned 8 bit integer and hence the check for a negative
value is always going to be false. Fix this by using ret as the error
return and hence the negative error can be detected and assign
the u8 sndif_format to ret if there is no error.
Detected by CoverityScan, CID#1469385 ("Unsigned compared against 0")
Signed-off-by: Colin Ian King
---
sound/xen/xen_snd_front_alsa.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/sound/xen/xen_snd_front_alsa.c b/sound/xen/xen_snd_front_alsa.c
index 5041f83e98d2..5a2bd70a2fa1 100644
--- a/sound/xen/xen_snd_front_alsa.c
+++ b/sound/xen/xen_snd_front_alsa.c
@@ -466,13 +466,14 @@ static int alsa_prepare(struct snd_pcm_substream
*substream)
u8 sndif_format;
int ret;
- sndif_format = to_sndif_format(runtime->format);
- if (sndif_format < 0) {
+ ret = to_sndif_format(runtime->format);
+ if (ret < 0) {
dev_err(>front_info->xb_dev->dev,
"Unsupported sample format: %d\n",
runtime->format);
- return sndif_format;
+ return ret;
}
+ sndif_format = ret;
ret = xen_snd_front_stream_prepare(>evt_pair->req,
>sh_buf,
Indeed. A typical assignment mistake. Instead, we could change the type
of 'sndif_format' to signed int, however in this case it's not the same
as the third argument of xen_snd_front_stream_prepare() because it is
'u8'. This patch looks good to me.
Reviewed-by: Takashi Sakamoto
Regards
Takashi Sakamoto
Re: [PATCH][next] ALSA: xen-front: remove redundant error check on ret
Hi,
On May 28 2018 06:23, Colin King wrote:
From: Colin Ian King <colin.k...@canonical.com>
The error for a -ve value in ret is redundant as all previous
assignments to ret have an associated -ve check and hence it
is impossible for ret to be less that zero at the point of the
check. Remove this redundant error check.
Detected by CoveritScan, CID#1469407 ("Logically Dead code")
Signed-off-by: Colin Ian King <colin.k...@canonical.com>
---
sound/xen/xen_snd_front_evtchnl.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sound/xen/xen_snd_front_evtchnl.c
b/sound/xen/xen_snd_front_evtchnl.c
index d70a62e7f910..102d6e096cc8 100644
--- a/sound/xen/xen_snd_front_evtchnl.c
+++ b/sound/xen/xen_snd_front_evtchnl.c
@@ -351,8 +351,6 @@ int xen_snd_front_evtchnl_create_all(struct
xen_snd_front_info *front_info,
}
}
}
- if (ret < 0)
- goto fail;
front_info->num_evt_pairs = num_streams;
return 0;
Yep. All branches for error path on the nested for loop have goto
statement, thus no need to check error outer the loop.
Reviewed-by: Takashi Sakamoto <o-taka...@sakamocchi.jp>
Takashi Sakamoto
Re: [PATCH][next] ALSA: xen-front: remove redundant error check on ret
Hi,
On May 28 2018 06:23, Colin King wrote:
From: Colin Ian King
The error for a -ve value in ret is redundant as all previous
assignments to ret have an associated -ve check and hence it
is impossible for ret to be less that zero at the point of the
check. Remove this redundant error check.
Detected by CoveritScan, CID#1469407 ("Logically Dead code")
Signed-off-by: Colin Ian King
---
sound/xen/xen_snd_front_evtchnl.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sound/xen/xen_snd_front_evtchnl.c
b/sound/xen/xen_snd_front_evtchnl.c
index d70a62e7f910..102d6e096cc8 100644
--- a/sound/xen/xen_snd_front_evtchnl.c
+++ b/sound/xen/xen_snd_front_evtchnl.c
@@ -351,8 +351,6 @@ int xen_snd_front_evtchnl_create_all(struct
xen_snd_front_info *front_info,
}
}
}
- if (ret < 0)
- goto fail;
front_info->num_evt_pairs = num_streams;
return 0;
Yep. All branches for error path on the nested for loop have goto
statement, thus no need to check error outer the loop.
Reviewed-by: Takashi Sakamoto
Takashi Sakamoto
Re: [PATCH v3 5/6] ALSA: xen-front: Implement ALSA virtual sound driver
Hi,
On May 14 2018 15:27, Oleksandr Andrushchenko wrote:
> diff --git a/sound/xen/xen_snd_front_alsa.c
b/sound/xen/xen_snd_front_alsa.c
> new file mode 100644
> index ..5041f83e98d2
> --- /dev/null
> +++ b/sound/xen/xen_snd_front_alsa.c
> ...
> +/*
> + * FIXME: The mmaped data transfer is asynchronous and there is no
> + * ack signal from user-space when it is done. This is the
> + * reason it is not implemented in the PV driver as we do need
> + * to know when the buffer can be transferred to the backend.
> + */
> ...
In ALSA PCM interface v2.0.14 or later, SNDRV_PCM_INFO_SYNC_APPLPTR is
available. This flag express that userspace applications are expected to
call ioctl(2) with 'SNDRV_PCM_IOCTL_SYNC_PTR' even if they oprate to
memory mapped page frames for PCM samples. For detail, please refer to a
commit 42f945970af9 ('ALSA: pcm: Add the explicit appl_ptr sync
support')[1].
As a supplement, please refer to below commits:
* 4b671f577474 ('ALSA: pcm: Add an ioctl to specify the supported
protocol version')[2]
* b602aa8eb1a0 ('ALSA: pcm: Disable only control mmap for explicit
appl_ptr sync')[3]
Alsa-lib v1.1.5 or later supports this flag and existent applications
can run transparently.
For developers, a tracepoint may be useful:
* fccf53881e9b ('ALSA: pcm: add 'applptr' event of tracepoint')[4]
For your information.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=42f945970af9
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4b671f577474
[3]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b602aa8eb1a0
[4]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fccf53881e9b
Regards
Takashi Sakamoto
Re: [PATCH v3 5/6] ALSA: xen-front: Implement ALSA virtual sound driver
Hi,
On May 14 2018 15:27, Oleksandr Andrushchenko wrote:
> diff --git a/sound/xen/xen_snd_front_alsa.c
b/sound/xen/xen_snd_front_alsa.c
> new file mode 100644
> index ..5041f83e98d2
> --- /dev/null
> +++ b/sound/xen/xen_snd_front_alsa.c
> ...
> +/*
> + * FIXME: The mmaped data transfer is asynchronous and there is no
> + * ack signal from user-space when it is done. This is the
> + * reason it is not implemented in the PV driver as we do need
> + * to know when the buffer can be transferred to the backend.
> + */
> ...
In ALSA PCM interface v2.0.14 or later, SNDRV_PCM_INFO_SYNC_APPLPTR is
available. This flag express that userspace applications are expected to
call ioctl(2) with 'SNDRV_PCM_IOCTL_SYNC_PTR' even if they oprate to
memory mapped page frames for PCM samples. For detail, please refer to a
commit 42f945970af9 ('ALSA: pcm: Add the explicit appl_ptr sync
support')[1].
As a supplement, please refer to below commits:
* 4b671f577474 ('ALSA: pcm: Add an ioctl to specify the supported
protocol version')[2]
* b602aa8eb1a0 ('ALSA: pcm: Disable only control mmap for explicit
appl_ptr sync')[3]
Alsa-lib v1.1.5 or later supports this flag and existent applications
can run transparently.
For developers, a tracepoint may be useful:
* fccf53881e9b ('ALSA: pcm: add 'applptr' event of tracepoint')[4]
For your information.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=42f945970af9
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4b671f577474
[3]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b602aa8eb1a0
[4]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fccf53881e9b
Regards
Takashi Sakamoto
Re: [PATCH 28/33] ALSA: dice use match_string() helper
Hi,
On May 21 2018 20:58, Yisheng Xie wrote:
match_string() returns the index of an array for a matching string,
which can be used intead of open coded variant.
Cc: Clemens Ladisch <clem...@ladisch.de>
Cc: Jaroslav Kysela <pe...@perex.cz>
Cc: Takashi Iwai <ti...@suse.com>
Cc: Takashi Sakamoto <o-taka...@sakamocchi.jp>
Cc: alsa-de...@alsa-project.org
Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
---
sound/firewire/dice/dice.c | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
I'm welcome to this change, while in the latest subsystem tree for v4.18
the 'force_to_pcm_support()' local function was removed by my patch[1].
Your patch can be abandon.
diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c
index 96bb01b..0639074 100644
--- a/sound/firewire/dice/dice.c
+++ b/sound/firewire/dice/dice.c
@@ -35,19 +35,13 @@ static bool force_two_pcm_support(struct fw_unit *unit)
"SAFFIRE_PRO_40_1",
};
char model[32];
- unsigned int i;
int err;
err = fw_csr_string(unit->directory, CSR_MODEL, model, sizeof(model));
if (err < 0)
return false;
- for (i = 0; i < ARRAY_SIZE(models); i++) {
- if (strcmp(models[i], model) == 0)
- break;
- }
-
- return i < ARRAY_SIZE(models);
+ return match_string(models, ARRAY_SIZE(models), model) >= 0;
}
static int check_dice_category(struct fw_unit *unit)
[1] ALSA: dice: remove local frag of force_two_pcms
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?h=for-next=9c367c01d3d5
Thanks
Takashi Sakamoto
Re: [PATCH 28/33] ALSA: dice use match_string() helper
Hi,
On May 21 2018 20:58, Yisheng Xie wrote:
match_string() returns the index of an array for a matching string,
which can be used intead of open coded variant.
Cc: Clemens Ladisch
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Cc: Takashi Sakamoto
Cc: alsa-de...@alsa-project.org
Signed-off-by: Yisheng Xie
---
sound/firewire/dice/dice.c | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
I'm welcome to this change, while in the latest subsystem tree for v4.18
the 'force_to_pcm_support()' local function was removed by my patch[1].
Your patch can be abandon.
diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c
index 96bb01b..0639074 100644
--- a/sound/firewire/dice/dice.c
+++ b/sound/firewire/dice/dice.c
@@ -35,19 +35,13 @@ static bool force_two_pcm_support(struct fw_unit *unit)
"SAFFIRE_PRO_40_1",
};
char model[32];
- unsigned int i;
int err;
err = fw_csr_string(unit->directory, CSR_MODEL, model, sizeof(model));
if (err < 0)
return false;
- for (i = 0; i < ARRAY_SIZE(models); i++) {
- if (strcmp(models[i], model) == 0)
- break;
- }
-
- return i < ARRAY_SIZE(models);
+ return match_string(models, ARRAY_SIZE(models), model) >= 0;
}
static int check_dice_category(struct fw_unit *unit)
[1] ALSA: dice: remove local frag of force_two_pcms
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?h=for-next=9c367c01d3d5
Thanks
Takashi Sakamoto
Re: [alsa-devel] [PATCH 29/33] ALSA: oxfw: use match_string() helper
Hi,
On May 21 2018 20:58, Yisheng Xie wrote:
match_string() returns the index of an array for a matching string,
which can be used intead of open coded variant.
Cc: Clemens Ladisch <clem...@ladisch.de>
Cc: Jaroslav Kysela <pe...@perex.cz>
Cc: Takashi Iwai <ti...@suse.com>
Cc: alsa-de...@alsa-project.org
Signed-off-by: Yisheng Xie <xieyishe...@huawei.com>
---
sound/firewire/oxfw/oxfw.c | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
Reviewed-by: Takashi Sakamoto <o-taka...@sakamocchi.jp>
For my information, use_match_string() helper was firstly introduced to
v4.6 kernel by a commit 56b060814e2d ('lib/string: introduce
match_string() helper').
diff --git a/sound/firewire/oxfw/oxfw.c b/sound/firewire/oxfw/oxfw.c
index 413ab63..1e5b2c8 100644
--- a/sound/firewire/oxfw/oxfw.c
+++ b/sound/firewire/oxfw/oxfw.c
@@ -49,7 +49,6 @@ static bool detect_loud_models(struct fw_unit *unit)
"Tapco LINK.firewire 4x6",
"U.420"};
char model[32];
- unsigned int i;
int err;
err = fw_csr_string(unit->directory, CSR_MODEL,
@@ -57,12 +56,7 @@ static bool detect_loud_models(struct fw_unit *unit)
if (err < 0)
return false;
- for (i = 0; i < ARRAY_SIZE(models); i++) {
- if (strcmp(models[i], model) == 0)
- break;
- }
-
- return (i < ARRAY_SIZE(models));
+ return match_string(models, ARRAY_SIZE(models), model) >= 0;
}
static int name_card(struct snd_oxfw *oxfw)
Thanks
Takashi Sakamoto
Re: [alsa-devel] [PATCH 29/33] ALSA: oxfw: use match_string() helper
Hi,
On May 21 2018 20:58, Yisheng Xie wrote:
match_string() returns the index of an array for a matching string,
which can be used intead of open coded variant.
Cc: Clemens Ladisch
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Cc: alsa-de...@alsa-project.org
Signed-off-by: Yisheng Xie
---
sound/firewire/oxfw/oxfw.c | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
Reviewed-by: Takashi Sakamoto
For my information, use_match_string() helper was firstly introduced to
v4.6 kernel by a commit 56b060814e2d ('lib/string: introduce
match_string() helper').
diff --git a/sound/firewire/oxfw/oxfw.c b/sound/firewire/oxfw/oxfw.c
index 413ab63..1e5b2c8 100644
--- a/sound/firewire/oxfw/oxfw.c
+++ b/sound/firewire/oxfw/oxfw.c
@@ -49,7 +49,6 @@ static bool detect_loud_models(struct fw_unit *unit)
"Tapco LINK.firewire 4x6",
"U.420"};
char model[32];
- unsigned int i;
int err;
err = fw_csr_string(unit->directory, CSR_MODEL,
@@ -57,12 +56,7 @@ static bool detect_loud_models(struct fw_unit *unit)
if (err < 0)
return false;
- for (i = 0; i < ARRAY_SIZE(models); i++) {
- if (strcmp(models[i], model) == 0)
- break;
- }
-
- return (i < ARRAY_SIZE(models));
+ return match_string(models, ARRAY_SIZE(models), model) >= 0;
}
static int name_card(struct snd_oxfw *oxfw)
Thanks
Takashi Sakamoto
Re: [PATCH v5 00/13] ALSA: hda/ca0132: Patch Series for Recon3Di and Sound Blaster Z Support
Hi, On May 9 2018 02:20, Connor McAdams wrote: This patchset adds support for the Sound Blaster Z and the Recon3Di. In order to figure out how to get these cards to work, I made a program called QemuHDADump[1], which uses the trace function of qemu to see interactions with the memory mapped pci BAR space of the card being used in the virtual machine. With this, I obtain the CORB buffer location to get the command verbs, and then dump them each time the buffer rolls over. This program may be useful for fixing other HDA related driver issues where there is no documentation for the device. So far, I have been able to get all features supported on the Sound Blaster Z and the Recon3Di. All output and input effects work, all inputs and outputs work, and just about anything else I can think of. I have also added new controls in order to select the new inputs and outputs, as well as controls to change the effect levels and presets. I have also added the ability to use firmware taken from the Windows drivers of both the Sound Blaster Z and Recon3Di. I am trying to get into contact with Creative to get permission to redistribute these along with the current file included with the Chromebook, but they have not been very responsive. Luckily, the cards work with the Chromebook firmware just fine, although I believe there has to be a reason they have different firmware in Windows. I will not link to the firmwares here, but if you look up my thread on Creative Labs forums, you will find the link to download the firmwares there. I am willing to help get the other non-working cards such as the ZxR and the newer AE-5 working too, but I will need someone willing to run QemuHDADump in a virtual machine in order to get the commands. So, in summary: -This patchset makes the cards work better than they did before (they really didn't work before) -This patchset leaves the original chromebook related stuff alone. Thanks. [1] https://github.com/Conmanx360/QemuHDADump Bugs: --- Recon3Di: (Reported by Mariusz Ceier) *** -Occasionally switching between rear and front mic breaks the input until computer is shutdown or put to sleep. -Surround Sound works, but is inconsistent. Sometimes, just updating the volume fixes it, and sometimes, it requires a restart. Sound Blaster Z: *** -none that I'm aware of. Version changes: --- v1: *** -Massive patch formatting failure, please ignore v1. v2: *** -Fixed patch formatting failure. v3: *** -Fixed mem_base unmap, instead of checking for QUIRK_SBZ on exit, have it check if the area is mapped, and if it is, unmap it. Also make it unmap after all other commands are finished. -Change notification of failure to map mem_base from codec_dbg to codec_warn, and use codec_info to tell the user that their card might have been incorrectly identified as a Sound Blaster Z. -Remove commented out commands in sbz_exit_chip function, only reintroduce them when their functions are defined. v4: *** -Split patch into smaller pieces. -Added const to alt_out_presets array. -Fixed command that was commented out and only put it in when it was actually used. v5: *** -Fixed issue identified by kbuild test robot, where patch 12 didn't compile individually. Connor McAdams (13): ALSA: hda/ca0132: R3Di and SBZ quirk entires + alt firmware loading ALSA: hda/ca0132: Add pincfg for SBZ + R3Di, add fp hp auto-detect ALSA: hda/ca0132: Add PCI region2 iomap for SBZ ALSA: hda/ca0132: Add extra exit functions for R3Di and SBZ ALSA: hda/ca0132: add extra init functions for r3di + sbz ALSA: hda/ca0132: update core functions for sbz + r3di ALSA: hda/ca0132: add dsp setup related commands for the sbz ALSA: hda/ca0132: Add dsp setup + gpio functions for r3di ALSA: hda/ca0132: add the ability to set src_id on scp commands ALSA: hda/ca0132: add alt_select_in/out for R3Di + SBZ ALSA: hda/ca0132: Add DSP Volume set and New mixers for SBZ + R3Di ALSA: hda/ca0132: add ca0132_alt_set_vipsource ALSA: hda/ca0132: Add new control changes for SBZ + R3Di sound/pci/hda/patch_ca0132.c | 3057 -- 1 file changed, 2941 insertions(+), 116 deletions(-) I reviewed the above patches. Reviewed-by: Takashi Sakamoto <o-taka...@sakamocchi.jp> There're some points to improve such like read-only memory objects, and I'll post some patches for them after merging this patchset. Thanks Takashi Sakamoto
Re: [PATCH v5 00/13] ALSA: hda/ca0132: Patch Series for Recon3Di and Sound Blaster Z Support
Hi, On May 9 2018 02:20, Connor McAdams wrote: This patchset adds support for the Sound Blaster Z and the Recon3Di. In order to figure out how to get these cards to work, I made a program called QemuHDADump[1], which uses the trace function of qemu to see interactions with the memory mapped pci BAR space of the card being used in the virtual machine. With this, I obtain the CORB buffer location to get the command verbs, and then dump them each time the buffer rolls over. This program may be useful for fixing other HDA related driver issues where there is no documentation for the device. So far, I have been able to get all features supported on the Sound Blaster Z and the Recon3Di. All output and input effects work, all inputs and outputs work, and just about anything else I can think of. I have also added new controls in order to select the new inputs and outputs, as well as controls to change the effect levels and presets. I have also added the ability to use firmware taken from the Windows drivers of both the Sound Blaster Z and Recon3Di. I am trying to get into contact with Creative to get permission to redistribute these along with the current file included with the Chromebook, but they have not been very responsive. Luckily, the cards work with the Chromebook firmware just fine, although I believe there has to be a reason they have different firmware in Windows. I will not link to the firmwares here, but if you look up my thread on Creative Labs forums, you will find the link to download the firmwares there. I am willing to help get the other non-working cards such as the ZxR and the newer AE-5 working too, but I will need someone willing to run QemuHDADump in a virtual machine in order to get the commands. So, in summary: -This patchset makes the cards work better than they did before (they really didn't work before) -This patchset leaves the original chromebook related stuff alone. Thanks. [1] https://github.com/Conmanx360/QemuHDADump Bugs: --- Recon3Di: (Reported by Mariusz Ceier) *** -Occasionally switching between rear and front mic breaks the input until computer is shutdown or put to sleep. -Surround Sound works, but is inconsistent. Sometimes, just updating the volume fixes it, and sometimes, it requires a restart. Sound Blaster Z: *** -none that I'm aware of. Version changes: --- v1: *** -Massive patch formatting failure, please ignore v1. v2: *** -Fixed patch formatting failure. v3: *** -Fixed mem_base unmap, instead of checking for QUIRK_SBZ on exit, have it check if the area is mapped, and if it is, unmap it. Also make it unmap after all other commands are finished. -Change notification of failure to map mem_base from codec_dbg to codec_warn, and use codec_info to tell the user that their card might have been incorrectly identified as a Sound Blaster Z. -Remove commented out commands in sbz_exit_chip function, only reintroduce them when their functions are defined. v4: *** -Split patch into smaller pieces. -Added const to alt_out_presets array. -Fixed command that was commented out and only put it in when it was actually used. v5: *** -Fixed issue identified by kbuild test robot, where patch 12 didn't compile individually. Connor McAdams (13): ALSA: hda/ca0132: R3Di and SBZ quirk entires + alt firmware loading ALSA: hda/ca0132: Add pincfg for SBZ + R3Di, add fp hp auto-detect ALSA: hda/ca0132: Add PCI region2 iomap for SBZ ALSA: hda/ca0132: Add extra exit functions for R3Di and SBZ ALSA: hda/ca0132: add extra init functions for r3di + sbz ALSA: hda/ca0132: update core functions for sbz + r3di ALSA: hda/ca0132: add dsp setup related commands for the sbz ALSA: hda/ca0132: Add dsp setup + gpio functions for r3di ALSA: hda/ca0132: add the ability to set src_id on scp commands ALSA: hda/ca0132: add alt_select_in/out for R3Di + SBZ ALSA: hda/ca0132: Add DSP Volume set and New mixers for SBZ + R3Di ALSA: hda/ca0132: add ca0132_alt_set_vipsource ALSA: hda/ca0132: Add new control changes for SBZ + R3Di sound/pci/hda/patch_ca0132.c | 3057 -- 1 file changed, 2941 insertions(+), 116 deletions(-) I reviewed the above patches. Reviewed-by: Takashi Sakamoto There're some points to improve such like read-only memory objects, and I'll post some patches for them after merging this patchset. Thanks Takashi Sakamoto
Re: [PATCH v3 6/9] ALSA: hda/ca0132: add alt_select_in/out for R3Di + SBZ
Hi,
On May 6 2018 04:03, Connor McAdams wrote:
Add functions ca0132_alt_select_out and ca0132_alt_select_in for
switching outputs and inputs for r3di and sbz. Also, add enumerated
controls for selecting output and input source.
Signed-off-by: Connor McAdams <conmanx...@gmail.com>
---
sound/pci/hda/patch_ca0132.c | 597 +--
1 file changed, 572 insertions(+), 25 deletions(-)
diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index bb0feaa..36cc2a1 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
@@ -480,6 +494,49 @@ static struct ct_voicefx_preset ca0132_voicefx_presets[] =
{
}
};
+/* DSP command sequences for ca0132_alt_select_out */
+#define ALT_OUT_SET_MAX_COMMANDS 9 /* Max number of commands in sequence */
+struct ca0132_alt_out_set {
+ char *name; /*preset name*/
+ unsigned char commands;
+ unsigned int mids[ALT_OUT_SET_MAX_COMMANDS];
+ unsigned int reqs[ALT_OUT_SET_MAX_COMMANDS];
+ unsigned int vals[ALT_OUT_SET_MAX_COMMANDS];
+};
+
+static struct ca0132_alt_out_set alt_out_presets[] = {
+ { .name = "Line Out",
+ .commands = 7,
+ .mids = { 0x96, 0x96, 0x96, 0x8F,
+ 0x96, 0x96, 0x96 },
+ .reqs = { 0x19, 0x17, 0x18, 0x01,
+ 0x1F, 0x15, 0x3A },
+ .vals = { 0x3F00, 0x42A0, 0x,
+ 0x, 0x, 0x,
+ 0x }
+ },
+ { .name = "Headphone",
+ .commands = 7,
+ .mids = { 0x96, 0x96, 0x96, 0x8F,
+ 0x96, 0x96, 0x96 },
+ .reqs = { 0x19, 0x17, 0x18, 0x01,
+ 0x1F, 0x15, 0x3A },
+ .vals = { 0x3F00, 0x42A0, 0x,
+ 0x, 0x, 0x,
+ 0x }
+ },
+ { .name = "Surround",
+ .commands = 8,
+ .mids = { 0x96, 0x8F, 0x96, 0x96,
+ 0x96, 0x96, 0x96, 0x96 },
+ .reqs = { 0x18, 0x01, 0x1F, 0x15,
+ 0x3A, 0x1A, 0x1B, 0x1C },
+ .vals = { 0x, 0x, 0x,
+ 0x, 0x, 0x,
+ 0x, 0x }
+ }
+};
+
It's better to add 'const' qualifier.
Regards
Takashi Sakamoto
Re: [PATCH v3 6/9] ALSA: hda/ca0132: add alt_select_in/out for R3Di + SBZ
Hi,
On May 6 2018 04:03, Connor McAdams wrote:
Add functions ca0132_alt_select_out and ca0132_alt_select_in for
switching outputs and inputs for r3di and sbz. Also, add enumerated
controls for selecting output and input source.
Signed-off-by: Connor McAdams
---
sound/pci/hda/patch_ca0132.c | 597 +--
1 file changed, 572 insertions(+), 25 deletions(-)
diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index bb0feaa..36cc2a1 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
@@ -480,6 +494,49 @@ static struct ct_voicefx_preset ca0132_voicefx_presets[] =
{
}
};
+/* DSP command sequences for ca0132_alt_select_out */
+#define ALT_OUT_SET_MAX_COMMANDS 9 /* Max number of commands in sequence */
+struct ca0132_alt_out_set {
+ char *name; /*preset name*/
+ unsigned char commands;
+ unsigned int mids[ALT_OUT_SET_MAX_COMMANDS];
+ unsigned int reqs[ALT_OUT_SET_MAX_COMMANDS];
+ unsigned int vals[ALT_OUT_SET_MAX_COMMANDS];
+};
+
+static struct ca0132_alt_out_set alt_out_presets[] = {
+ { .name = "Line Out",
+ .commands = 7,
+ .mids = { 0x96, 0x96, 0x96, 0x8F,
+ 0x96, 0x96, 0x96 },
+ .reqs = { 0x19, 0x17, 0x18, 0x01,
+ 0x1F, 0x15, 0x3A },
+ .vals = { 0x3F00, 0x42A0, 0x,
+ 0x, 0x, 0x,
+ 0x }
+ },
+ { .name = "Headphone",
+ .commands = 7,
+ .mids = { 0x96, 0x96, 0x96, 0x8F,
+ 0x96, 0x96, 0x96 },
+ .reqs = { 0x19, 0x17, 0x18, 0x01,
+ 0x1F, 0x15, 0x3A },
+ .vals = { 0x3F00, 0x42A0, 0x,
+ 0x, 0x, 0x,
+ 0x }
+ },
+ { .name = "Surround",
+ .commands = 8,
+ .mids = { 0x96, 0x8F, 0x96, 0x96,
+ 0x96, 0x96, 0x96, 0x96 },
+ .reqs = { 0x18, 0x01, 0x1F, 0x15,
+ 0x3A, 0x1A, 0x1B, 0x1C },
+ .vals = { 0x, 0x, 0x,
+ 0x, 0x, 0x,
+ 0x, 0x }
+ }
+};
+
It's better to add 'const' qualifier.
Regards
Takashi Sakamoto
Re: [PATCH v3 5/9] ALSA: hda/ca0132: add/change helper functions for R3Di and SBZ
Hi, On May 6 2018 04:03, Connor McAdams wrote: Edit core functions to support the Sound Blaster Z and Recon3Di for startup and loading of the DSP, as well as setting effects. Signed-off-by: Connor McAdams <conmanx...@gmail.com> --- sound/pci/hda/patch_ca0132.c | 1064 -- 1 file changed, 1018 insertions(+), 46 deletions(-) In my opinion, this patch is too large. This patch can be split into several parts: * Changes for signature of 'dspio_scp()' to get 'src_id' * dspio_scp() * dspio_set_param() * dspio_set_uint_param() * dspio_alloc_dma_chan() * dspio_free_dma_chan() * Changes for SBZ only * Changes for R3Di only Could you please prepare for these three patches from this large patch in your next chance? Especially, you can describe enough information to the latter two patches as patch comment. Thanks Takashi Sakamoto
Re: [PATCH v3 5/9] ALSA: hda/ca0132: add/change helper functions for R3Di and SBZ
Hi, On May 6 2018 04:03, Connor McAdams wrote: Edit core functions to support the Sound Blaster Z and Recon3Di for startup and loading of the DSP, as well as setting effects. Signed-off-by: Connor McAdams --- sound/pci/hda/patch_ca0132.c | 1064 -- 1 file changed, 1018 insertions(+), 46 deletions(-) In my opinion, this patch is too large. This patch can be split into several parts: * Changes for signature of 'dspio_scp()' to get 'src_id' * dspio_scp() * dspio_set_param() * dspio_set_uint_param() * dspio_alloc_dma_chan() * dspio_free_dma_chan() * Changes for SBZ only * Changes for R3Di only Could you please prepare for these three patches from this large patch in your next chance? Especially, you can describe enough information to the latter two patches as patch comment. Thanks Takashi Sakamoto
Re: [PATCH 1/9] R3Di and SBZ quirk entires + alt firmware loading
Hi, On May 04 2018 13:30, Connor McAdams wrote: Sorry for sending these twice, I made a formatting mistake in the first series, and they would not apply properly. Hopefully these do not show up as spam because of this. I went through and fixed them all individually and re-committed them, but kept the same commit messages. I still have a lot to learn. For this case, please use '--subject-prefix' option for git-format-patch. Like: $ git format-patch --subject-prefix='PATCH v2' Additionally, it's preferable to add 'ALSA: hda/ca0132: ' prefix for each patch. Like: 'ALSA: hda/ca0132: R3Di and SBZ quirk entires + alt firmware loading' Furthermore, when posting several patches as a one series, it's preferable to post cover letter as well. Like: $ git format-patch --cover-letter -cover-letter.patch ... Of course, it's better to write enough information in the cover letter. For its content, please refer to cover letters by the other developers in alsa-devel archive. http://mailman.alsa-project.org/pipermail/alsa-devel/ Thanks Takashi Sakamoto
Re: [PATCH 1/9] R3Di and SBZ quirk entires + alt firmware loading
Hi, On May 04 2018 13:30, Connor McAdams wrote: Sorry for sending these twice, I made a formatting mistake in the first series, and they would not apply properly. Hopefully these do not show up as spam because of this. I went through and fixed them all individually and re-committed them, but kept the same commit messages. I still have a lot to learn. For this case, please use '--subject-prefix' option for git-format-patch. Like: $ git format-patch --subject-prefix='PATCH v2' Additionally, it's preferable to add 'ALSA: hda/ca0132: ' prefix for each patch. Like: 'ALSA: hda/ca0132: R3Di and SBZ quirk entires + alt firmware loading' Furthermore, when posting several patches as a one series, it's preferable to post cover letter as well. Like: $ git format-patch --cover-letter -cover-letter.patch ... Of course, it's better to write enough information in the cover letter. For its content, please refer to cover letters by the other developers in alsa-devel archive. http://mailman.alsa-project.org/pipermail/alsa-devel/ Thanks Takashi Sakamoto
Re: [PATCH v2] pcm_native: Remove VLA usage
Hi,
On Mar 29 2018 07:24, Kyle Spiers wrote:
As part of the effort to remove VLAs from the kernel[1], this changes
the allocation of the rstamps array from being on the stack to being
kcalloc()ed. This also allows for the removal of the explicit zeroing
loop.
Signed-off-by: Kyle Spiers <k...@spiers.me>
---
sound/core/pcm_native.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
This bug was already fixed at a commit 5730f9f744cf ('
ALSA: pcm: Remove VLA usage')[1][2].
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/sound/core/pcm_native.c?h=for-next=5730f9f744cfe20b771adc33f3b476b95d3eebba
[2]
http://mailman.alsa-project.org/pipermail/alsa-devel/2018-March/133268.html
Thanks
Takashi Sakamoto
Re: [PATCH v2] pcm_native: Remove VLA usage
Hi,
On Mar 29 2018 07:24, Kyle Spiers wrote:
As part of the effort to remove VLAs from the kernel[1], this changes
the allocation of the rstamps array from being on the stack to being
kcalloc()ed. This also allows for the removal of the explicit zeroing
loop.
Signed-off-by: Kyle Spiers
---
sound/core/pcm_native.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
This bug was already fixed at a commit 5730f9f744cf ('
ALSA: pcm: Remove VLA usage')[1][2].
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/sound/core/pcm_native.c?h=for-next=5730f9f744cfe20b771adc33f3b476b95d3eebba
[2]
http://mailman.alsa-project.org/pipermail/alsa-devel/2018-March/133268.html
Thanks
Takashi Sakamoto
Re: [PATCH] ALSA: pcm: Remove VLA usage
Hi,
On Mar 13 2018 20:37, Takashi Iwai wrote:
A helper function used by snd_pcm_hw_refine() still keeps using VLA
for timestamps of hw constraint rules that are non-fixed size.
Let's replace the VLA with a simple kmalloc() array.
Reference: https://lkml.org/lkml/2018/3/7/621
Signed-off-by: Takashi Iwai <ti...@suse.de>
---
sound/core/pcm_native.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
Yeah. I have concerned about it for a long time but kept it as is
because it had no issue. Now, let's obsolete it.
Reviewed-by: Takashi Sakamoto <o-taka...@sakamocchi.jp>
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 77ba50ddcf9e..756a9a3884a5 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -323,7 +323,7 @@ static int constrain_params_by_rules(struct
snd_pcm_substream *substream,
struct snd_pcm_hw_constraints *constrs =
>runtime->hw_constraints;
unsigned int k;
- unsigned int rstamps[constrs->rules_num];
+ unsigned int *rstamps;
unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1];
unsigned int stamp;
struct snd_pcm_hw_rule *r;
@@ -331,7 +331,7 @@ static int constrain_params_by_rules(struct
snd_pcm_substream *substream,
struct snd_mask old_mask;
struct snd_interval old_interval;
bool again;
- int changed;
+ int changed, err = 0;
/*
* Each application of rule has own sequence number.
@@ -339,8 +339,9 @@ static int constrain_params_by_rules(struct
snd_pcm_substream *substream,
* Each member of 'rstamps' array represents the sequence number of
* recent application of corresponding rule.
*/
- for (k = 0; k < constrs->rules_num; k++)
- rstamps[k] = 0;
+ rstamps = kcalloc(constrs->rules_num, sizeof(unsigned int), GFP_KERNEL);
+ if (!rstamps)
+ return -ENOMEM;
/*
* Each member of 'vstamps' array represents the sequence number of
@@ -398,8 +399,10 @@ static int constrain_params_by_rules(struct
snd_pcm_substream *substream,
}
changed = r->func(params, r);
- if (changed < 0)
- return changed;
+ if (changed < 0) {
+ err = changed;
+ goto out;
+ }
/*
* When the parameter is changed, notify it to the caller
@@ -430,7 +433,9 @@ static int constrain_params_by_rules(struct
snd_pcm_substream *substream,
if (again)
goto retry;
- return 0;
+ out:
+ kfree(rstamps);
+ return err;
}
static int fixup_unreferenced_params(struct snd_pcm_substream *substream,
Thanks
Takashi Sakamoto
Re: [PATCH] ALSA: pcm: Remove VLA usage
Hi,
On Mar 13 2018 20:37, Takashi Iwai wrote:
A helper function used by snd_pcm_hw_refine() still keeps using VLA
for timestamps of hw constraint rules that are non-fixed size.
Let's replace the VLA with a simple kmalloc() array.
Reference: https://lkml.org/lkml/2018/3/7/621
Signed-off-by: Takashi Iwai
---
sound/core/pcm_native.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
Yeah. I have concerned about it for a long time but kept it as is
because it had no issue. Now, let's obsolete it.
Reviewed-by: Takashi Sakamoto
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 77ba50ddcf9e..756a9a3884a5 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -323,7 +323,7 @@ static int constrain_params_by_rules(struct
snd_pcm_substream *substream,
struct snd_pcm_hw_constraints *constrs =
>runtime->hw_constraints;
unsigned int k;
- unsigned int rstamps[constrs->rules_num];
+ unsigned int *rstamps;
unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1];
unsigned int stamp;
struct snd_pcm_hw_rule *r;
@@ -331,7 +331,7 @@ static int constrain_params_by_rules(struct
snd_pcm_substream *substream,
struct snd_mask old_mask;
struct snd_interval old_interval;
bool again;
- int changed;
+ int changed, err = 0;
/*
* Each application of rule has own sequence number.
@@ -339,8 +339,9 @@ static int constrain_params_by_rules(struct
snd_pcm_substream *substream,
* Each member of 'rstamps' array represents the sequence number of
* recent application of corresponding rule.
*/
- for (k = 0; k < constrs->rules_num; k++)
- rstamps[k] = 0;
+ rstamps = kcalloc(constrs->rules_num, sizeof(unsigned int), GFP_KERNEL);
+ if (!rstamps)
+ return -ENOMEM;
/*
* Each member of 'vstamps' array represents the sequence number of
@@ -398,8 +399,10 @@ static int constrain_params_by_rules(struct
snd_pcm_substream *substream,
}
changed = r->func(params, r);
- if (changed < 0)
- return changed;
+ if (changed < 0) {
+ err = changed;
+ goto out;
+ }
/*
* When the parameter is changed, notify it to the caller
@@ -430,7 +433,9 @@ static int constrain_params_by_rules(struct
snd_pcm_substream *substream,
if (again)
goto retry;
- return 0;
+ out:
+ kfree(rstamps);
+ return err;
}
static int fixup_unreferenced_params(struct snd_pcm_substream *substream,
Thanks
Takashi Sakamoto
Re: [PATCH 1/2] ASoC: topology: Rename clock_gated to clock_cont in snd_soc_tplg_hw_config
Hi,
On Feb 19 2018 15:05, Kirill Marinushkin wrote:
In kernel `soc-dai.h`, DAI clock gating is defined as following:
\#define SND_SOC_DAIFMT_CONT(1 << 4) /* continuous clock */
\#define SND_SOC_DAIFMT_GATED (0 << 4) /* clock is gated */
Therefore, the corresponding field of struct snd_soc_tplg_hw_config should
be inverted compared to the current logic:
clock_count = 1 => SND_SOC_DAIFMT_CONT
clock_count = 0 => SND_SOC_DAIFMT_GATED
Signed-off-by: Kirill Marinushkin <k.marinush...@gmail.com>
Cc: Jaroslav Kysela <pe...@perex.cz>
Cc: Takashi Iwai <ti...@suse.com>
Cc: alsa-de...@alsa-project.org
Cc: linux-kernel@vger.kernel.org
---
include/uapi/sound/asoc.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/uapi/sound/asoc.h b/include/uapi/sound/asoc.h
index 69c37ecbff7e..10188850dede 100644
--- a/include/uapi/sound/asoc.h
+++ b/include/uapi/sound/asoc.h
@@ -312,7 +312,9 @@ struct snd_soc_tplg_hw_config {
__le32 size;/* in bytes of this structure */
__le32 id; /* unique ID - - used to match */
__le32 fmt; /* SND_SOC_DAI_FORMAT_ format value */
- __u8 clock_gated; /* 1 if clock can be gated to save power */
+ __u8 clock_cont;/* 1 if clock is continuous, and can not be
+* gated to save power
+*/
__u8 invert_bclk; /* 1 for inverted BCLK, 0 for normal */
__u8 invert_fsync; /* 1 for inverted frame clock, 0 for normal */
__u8 bclk_master; /* 1 for master of BCLK, 0 for slave */
This structure was added at a commit 676c6b5208f7 ('ASoC: topology: ABI
- Update physical DAI link configuration for version 5') in a
development period for v4.10.
This file is a part of UAPI, thus this structure has already been
exposed to application developers. Any change can break userspace
applications in a point of backward compatibility for this subsystem.
It's better for you to investigate another solution for your two
patches[1][2].
[1] [alsa-devel] [PATCH 1/2] ASoC: topology: Rename clock_gated to
clock_cont in snd_soc_tplg_hw_config
http://mailman.alsa-project.org/pipermail/alsa-devel/2018-February/132258.html
[2] [alsa-devel] [PATCH 2/2] ASoC: topology: Add missing clock gating
parameter when parsing hw_configs
http://mailman.alsa-project.org/pipermail/alsa-devel/2018-February/132259.html
Regards
Takashi Sakamoto
Re: [PATCH 1/2] ASoC: topology: Rename clock_gated to clock_cont in snd_soc_tplg_hw_config
Hi,
On Feb 19 2018 15:05, Kirill Marinushkin wrote:
In kernel `soc-dai.h`, DAI clock gating is defined as following:
\#define SND_SOC_DAIFMT_CONT(1 << 4) /* continuous clock */
\#define SND_SOC_DAIFMT_GATED (0 << 4) /* clock is gated */
Therefore, the corresponding field of struct snd_soc_tplg_hw_config should
be inverted compared to the current logic:
clock_count = 1 => SND_SOC_DAIFMT_CONT
clock_count = 0 => SND_SOC_DAIFMT_GATED
Signed-off-by: Kirill Marinushkin
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Cc: alsa-de...@alsa-project.org
Cc: linux-kernel@vger.kernel.org
---
include/uapi/sound/asoc.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/uapi/sound/asoc.h b/include/uapi/sound/asoc.h
index 69c37ecbff7e..10188850dede 100644
--- a/include/uapi/sound/asoc.h
+++ b/include/uapi/sound/asoc.h
@@ -312,7 +312,9 @@ struct snd_soc_tplg_hw_config {
__le32 size;/* in bytes of this structure */
__le32 id; /* unique ID - - used to match */
__le32 fmt; /* SND_SOC_DAI_FORMAT_ format value */
- __u8 clock_gated; /* 1 if clock can be gated to save power */
+ __u8 clock_cont;/* 1 if clock is continuous, and can not be
+* gated to save power
+*/
__u8 invert_bclk; /* 1 for inverted BCLK, 0 for normal */
__u8 invert_fsync; /* 1 for inverted frame clock, 0 for normal */
__u8 bclk_master; /* 1 for master of BCLK, 0 for slave */
This structure was added at a commit 676c6b5208f7 ('ASoC: topology: ABI
- Update physical DAI link configuration for version 5') in a
development period for v4.10.
This file is a part of UAPI, thus this structure has already been
exposed to application developers. Any change can break userspace
applications in a point of backward compatibility for this subsystem.
It's better for you to investigate another solution for your two
patches[1][2].
[1] [alsa-devel] [PATCH 1/2] ASoC: topology: Rename clock_gated to
clock_cont in snd_soc_tplg_hw_config
http://mailman.alsa-project.org/pipermail/alsa-devel/2018-February/132258.html
[2] [alsa-devel] [PATCH 2/2] ASoC: topology: Add missing clock gating
parameter when parsing hw_configs
http://mailman.alsa-project.org/pipermail/alsa-devel/2018-February/132259.html
Regards
Takashi Sakamoto
Re: [PATCH] ASoC: Intel: Skylake: Fix compiler warning -Wmaybe-uninitialized
Hi,
On Feb 19 2018 15:02, Kirill Marinushkin wrote:
Configuration:
SND_SOC_INTEL_SKYLAKE=y
PM_SLEEP=y
Warning message:
sound/soc/intel/skylake/skl.c: In function 'skl_resume':
sound/soc/intel/skylake/skl.c:326:6: warning: 'ret' may be used
uninitialized in this function [-Wmaybe-uninitialized]
int ret;
^~~
Fixes: 4557c305d4fc ("ASoC: Intel: Skylake: Add support for active suspend")
Signed-off-by: Kirill Marinushkin <k.marinush...@gmail.com>
Cc: Liam Girdwood <lgirdw...@gmail.com>
Cc: Mark Brown <broo...@kernel.org>
Cc: Jaroslav Kysela <pe...@perex.cz>
Cc: Takashi Iwai <ti...@suse.com>
Cc: Vinod Koul <vinod.k...@intel.com>
Cc: Guneshwor Singh <guneshwor.o.si...@intel.com>
Cc: Naveen Manohar <navee...@intel.com>
Cc: Harsha Priya N <harshapriy...@intel.com>
Cc: Pierre-Louis Bossart <pierre-louis.boss...@linux.intel.com>
Cc: alsa-de...@alsa-project.org
Cc: linux-kernel@vger.kernel.org
---
sound/soc/intel/skylake/skl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 31d8634e8aa1..273a9ab75489 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -323,7 +323,7 @@ static int skl_resume(struct device *dev)
struct skl *skl = ebus_to_skl(ebus);
struct hdac_bus *bus = ebus_to_hbus(ebus);
struct hdac_ext_link *hlink = NULL;
- int ret;
+ int ret = 0;
/* Turned OFF in HDMI codec driver after codec reconfiguration */
if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
At current HEAD (ea954fbc35e6) in 'topic/intel' branch[1] of Mark's
tree, this issue was already fixed by Arnd Bergmann Nov 2017 by his
commit cc20c4df1627 ('ASoC: intel: initialize return value properly')[2].
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/log/?h=topic/intel
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=topic/intel=cc20c4df1627
Regards
Takashi Sakamoto
Re: [PATCH] ASoC: Intel: Skylake: Fix compiler warning -Wmaybe-uninitialized
Hi,
On Feb 19 2018 15:02, Kirill Marinushkin wrote:
Configuration:
SND_SOC_INTEL_SKYLAKE=y
PM_SLEEP=y
Warning message:
sound/soc/intel/skylake/skl.c: In function 'skl_resume':
sound/soc/intel/skylake/skl.c:326:6: warning: 'ret' may be used
uninitialized in this function [-Wmaybe-uninitialized]
int ret;
^~~
Fixes: 4557c305d4fc ("ASoC: Intel: Skylake: Add support for active suspend")
Signed-off-by: Kirill Marinushkin
Cc: Liam Girdwood
Cc: Mark Brown
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Cc: Vinod Koul
Cc: Guneshwor Singh
Cc: Naveen Manohar
Cc: Harsha Priya N
Cc: Pierre-Louis Bossart
Cc: alsa-de...@alsa-project.org
Cc: linux-kernel@vger.kernel.org
---
sound/soc/intel/skylake/skl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 31d8634e8aa1..273a9ab75489 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -323,7 +323,7 @@ static int skl_resume(struct device *dev)
struct skl *skl = ebus_to_skl(ebus);
struct hdac_bus *bus = ebus_to_hbus(ebus);
struct hdac_ext_link *hlink = NULL;
- int ret;
+ int ret = 0;
/* Turned OFF in HDMI codec driver after codec reconfiguration */
if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
At current HEAD (ea954fbc35e6) in 'topic/intel' branch[1] of Mark's
tree, this issue was already fixed by Arnd Bergmann Nov 2017 by his
commit cc20c4df1627 ('ASoC: intel: initialize return value properly')[2].
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/log/?h=topic/intel
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?h=topic/intel=cc20c4df1627
Regards
Takashi Sakamoto
Re: [PATCH] ASoC: Intel: Skylake: make function skl_clk_round_rate static
Hi,
On Feb 8 2018 23:35, Colin King wrote:
From: Colin Ian King <colin.k...@canonical.com>
The function skl_clk_round_rate is local to the source and does not
need to be in global scope, so make it static.
Cleans up sparse warning:
sound/soc/intel/skylake/skl-ssp-clk.c:250:6: warning: symbol
'skl_clk_round_rate' was not declared. Should it be static?
Signed-off-by: Colin Ian King <colin.k...@canonical.com>
---
sound/soc/intel/skylake/skl-ssp-clk.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Takashi Sakamoto <o-taka...@sakamocchi.jp>
diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c
b/sound/soc/intel/skylake/skl-ssp-clk.c
index 7fbddf5e3b00..cda1b5fa7436 100644
--- a/sound/soc/intel/skylake/skl-ssp-clk.c
+++ b/sound/soc/intel/skylake/skl-ssp-clk.c
@@ -247,8 +247,8 @@ static unsigned long skl_clk_recalc_rate(struct clk_hw *hw,
}
/* Not supported by clk driver. Implemented to satisfy clk fw */
-long skl_clk_round_rate(struct clk_hw *hw, unsigned long rate,
- unsigned long *parent_rate)
+static long skl_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *parent_rate)
{
return rate;
}
Thanks
Takashi Sakamoto
Re: [PATCH] ASoC: Intel: Skylake: make function skl_clk_round_rate static
Hi,
On Feb 8 2018 23:35, Colin King wrote:
From: Colin Ian King
The function skl_clk_round_rate is local to the source and does not
need to be in global scope, so make it static.
Cleans up sparse warning:
sound/soc/intel/skylake/skl-ssp-clk.c:250:6: warning: symbol
'skl_clk_round_rate' was not declared. Should it be static?
Signed-off-by: Colin Ian King
---
sound/soc/intel/skylake/skl-ssp-clk.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Takashi Sakamoto
diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c
b/sound/soc/intel/skylake/skl-ssp-clk.c
index 7fbddf5e3b00..cda1b5fa7436 100644
--- a/sound/soc/intel/skylake/skl-ssp-clk.c
+++ b/sound/soc/intel/skylake/skl-ssp-clk.c
@@ -247,8 +247,8 @@ static unsigned long skl_clk_recalc_rate(struct clk_hw *hw,
}
/* Not supported by clk driver. Implemented to satisfy clk fw */
-long skl_clk_round_rate(struct clk_hw *hw, unsigned long rate,
- unsigned long *parent_rate)
+static long skl_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *parent_rate)
{
return rate;
}
Thanks
Takashi Sakamoto
Re: Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree
Hi Jiada,
On Jan 9 2018 14:42, Jiada Wang wrote:
Only the first patch in
https://www.spinics.net/lists/alsa-devel/msg71021.html
was applied, but the second patch "ASoC: rsnd: ssi: remove unnesessary
period_pos"
was missed
Could you please have a look
I can see all of them were applied to Mark's for-next branch[1].
- 33f801366bdf ('ASoC: rsnd: ssi: fix race condition in
rsnd_ssi_pointer_update')[2]
- 2e2d53da81af ('ASoC: rsnd: ssi: remove unnesessary period_pos')[3]
No worries to us.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/log/?h=for-next
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/sound/soc/sh?h=for-next=33f801366bdf3f8b67dfe325b84f4051a090d01e
[3]
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/sound/soc/sh?h=for-next=2e2d53da81af6bc6b4e025a5d01b37b4449b
Regards
Takashi Sakamoto
Re: Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree
Hi Jiada,
On Jan 9 2018 14:42, Jiada Wang wrote:
Only the first patch in
https://www.spinics.net/lists/alsa-devel/msg71021.html
was applied, but the second patch "ASoC: rsnd: ssi: remove unnesessary
period_pos"
was missed
Could you please have a look
I can see all of them were applied to Mark's for-next branch[1].
- 33f801366bdf ('ASoC: rsnd: ssi: fix race condition in
rsnd_ssi_pointer_update')[2]
- 2e2d53da81af ('ASoC: rsnd: ssi: remove unnesessary period_pos')[3]
No worries to us.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/log/?h=for-next
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/sound/soc/sh?h=for-next=33f801366bdf3f8b67dfe325b84f4051a090d01e
[3]
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/sound/soc/sh?h=for-next=2e2d53da81af6bc6b4e025a5d01b37b4449b
Regards
Takashi Sakamoto
Re: Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree
Hi Mark,
On Dec 9 2017 03:52, Mark Brown wrote:
The patch
ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
The applied patches are in v1 patchset, but we have v2 patchset already:
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-December/128970.html
Would you please replace the applied patches with the renewed ones?
There're slight differences between them.
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From 33f801366bdf3f8b67dfe325b84f4051a090d01e Mon Sep 17 00:00:00 2001
From: Jiada Wang <jiada_w...@mentor.com>
Date: Thu, 7 Dec 2017 22:15:38 -0800
Subject: [PATCH] ASoC: rsnd: ssi: fix race condition in
rsnd_ssi_pointer_update
Currently there is race condition between set of byte_pos and wrap
it around when new buffer starts. If .pointer is called in-between
it will result in inconsistent pointer position be returned
from .pointer callback.
This patch increments buffer pointer atomically to avoid this issue.
Signed-off-by: Jiada Wang <jiada_w...@mentor.com>
Reviewed-by: Takashi Sakamoto <takashi.sakam...@miraclelinux.com>
Acked-by: Kuninori Morimoto <kuninori.morimoto...@renesas.com>
Signed-off-by: Mark Brown <broo...@kernel.org>
---
sound/soc/sh/rcar/ssi.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c
index fece1e5f582f..cbf3bf312d23 100644
--- a/sound/soc/sh/rcar/ssi.c
+++ b/sound/soc/sh/rcar/ssi.c
@@ -446,25 +446,29 @@ static bool rsnd_ssi_pointer_update(struct rsnd_mod *mod,
int byte)
{
struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
+ bool ret = false;
+ int byte_pos;
- ssi->byte_pos += byte;
+ byte_pos = ssi->byte_pos + byte;
- if (ssi->byte_pos >= ssi->next_period_byte) {
+ if (byte_pos >= ssi->next_period_byte) {
struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io);
ssi->period_pos++;
ssi->next_period_byte += ssi->byte_per_period;
if (ssi->period_pos >= runtime->periods) {
- ssi->byte_pos = 0;
+ byte_pos = 0;
ssi->period_pos = 0;
ssi->next_period_byte = ssi->byte_per_period;
}
- return true;
+ ret = true;
}
- return false;
+ WRITE_ONCE(ssi->byte_pos, byte_pos);
+
+ return ret;
}
/*
@@ -838,7 +842,7 @@ static int rsnd_ssi_pointer(struct rsnd_mod *mod,
struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io);
- *pointer = bytes_to_frames(runtime, ssi->byte_pos);
+ *pointer = bytes_to_frames(runtime, READ_ONCE(ssi->byte_pos));
return 0;
}
Thanks
Takashi Sakamoto
Re: Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree
Hi Mark,
On Dec 9 2017 03:52, Mark Brown wrote:
The patch
ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
The applied patches are in v1 patchset, but we have v2 patchset already:
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-December/128970.html
Would you please replace the applied patches with the renewed ones?
There're slight differences between them.
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From 33f801366bdf3f8b67dfe325b84f4051a090d01e Mon Sep 17 00:00:00 2001
From: Jiada Wang
Date: Thu, 7 Dec 2017 22:15:38 -0800
Subject: [PATCH] ASoC: rsnd: ssi: fix race condition in
rsnd_ssi_pointer_update
Currently there is race condition between set of byte_pos and wrap
it around when new buffer starts. If .pointer is called in-between
it will result in inconsistent pointer position be returned
from .pointer callback.
This patch increments buffer pointer atomically to avoid this issue.
Signed-off-by: Jiada Wang
Reviewed-by: Takashi Sakamoto
Acked-by: Kuninori Morimoto
Signed-off-by: Mark Brown
---
sound/soc/sh/rcar/ssi.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c
index fece1e5f582f..cbf3bf312d23 100644
--- a/sound/soc/sh/rcar/ssi.c
+++ b/sound/soc/sh/rcar/ssi.c
@@ -446,25 +446,29 @@ static bool rsnd_ssi_pointer_update(struct rsnd_mod *mod,
int byte)
{
struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
+ bool ret = false;
+ int byte_pos;
- ssi->byte_pos += byte;
+ byte_pos = ssi->byte_pos + byte;
- if (ssi->byte_pos >= ssi->next_period_byte) {
+ if (byte_pos >= ssi->next_period_byte) {
struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io);
ssi->period_pos++;
ssi->next_period_byte += ssi->byte_per_period;
if (ssi->period_pos >= runtime->periods) {
- ssi->byte_pos = 0;
+ byte_pos = 0;
ssi->period_pos = 0;
ssi->next_period_byte = ssi->byte_per_period;
}
- return true;
+ ret = true;
}
- return false;
+ WRITE_ONCE(ssi->byte_pos, byte_pos);
+
+ return ret;
}
/*
@@ -838,7 +842,7 @@ static int rsnd_ssi_pointer(struct rsnd_mod *mod,
struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io);
- *pointer = bytes_to_frames(runtime, ssi->byte_pos);
+ *pointer = bytes_to_frames(runtime, READ_ONCE(ssi->byte_pos));
return 0;
}
Thanks
Takashi Sakamoto
Re: [PATCH] ALSA: drivers: make array 'names' const, reduces object code size
On Nov 28 2017 04:51, Takashi Iwai wrote:
On Mon, 27 Nov 2017 18:34:17 +0100,
Takashi Sakamoto wrote:
Hi,
On Nov 27 2017 21:58, Colin King wrote:
From: Colin Ian King <colin.k...@canonical.com>
Don't populate array 'names' on the stack but instead make them static.
Makes the object code smaller by 50 bytes:
Before:
text data bss dec hex filename
21237 91921120 315497b3d linux/sound/drivers/dummy.o
After:
text data bss dec hex filename
21095 92801120 314957b07 linux/sound/drivers/dummy.o
(gcc version 7.2.0 x86_64)
Signed-off-by: Colin Ian King <colin.k...@canonical.com>
---
sound/drivers/dummy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index 7b2b1f766b00..69db45bc0197 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -830,7 +830,7 @@ static int snd_dummy_capsrc_put(struct snd_kcontrol
*kcontrol, struct snd_ctl_el
static int snd_dummy_iobox_info(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_info *info)
{
- const char *const names[] = { "None", "CD Player" };
+ static const char *const names[] = { "None", "CD Player" };
return snd_ctl_enum_info(info, 1, 2, names);
}
Total size of snd-dummy.ko increases but this patch has an advantage
to have the symbol in read-only section.
The total size decreases :)
Ah, yes. The state of my local build tree is not good to calculate it...
In fact, the relocatable object decreases its size when built with a
header package from Ubuntu repository.
Thanks
Takashi Sakamoto
Re: [PATCH] ALSA: drivers: make array 'names' const, reduces object code size
On Nov 28 2017 04:51, Takashi Iwai wrote:
On Mon, 27 Nov 2017 18:34:17 +0100,
Takashi Sakamoto wrote:
Hi,
On Nov 27 2017 21:58, Colin King wrote:
From: Colin Ian King
Don't populate array 'names' on the stack but instead make them static.
Makes the object code smaller by 50 bytes:
Before:
text data bss dec hex filename
21237 91921120 315497b3d linux/sound/drivers/dummy.o
After:
text data bss dec hex filename
21095 92801120 314957b07 linux/sound/drivers/dummy.o
(gcc version 7.2.0 x86_64)
Signed-off-by: Colin Ian King
---
sound/drivers/dummy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index 7b2b1f766b00..69db45bc0197 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -830,7 +830,7 @@ static int snd_dummy_capsrc_put(struct snd_kcontrol
*kcontrol, struct snd_ctl_el
static int snd_dummy_iobox_info(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_info *info)
{
- const char *const names[] = { "None", "CD Player" };
+ static const char *const names[] = { "None", "CD Player" };
return snd_ctl_enum_info(info, 1, 2, names);
}
Total size of snd-dummy.ko increases but this patch has an advantage
to have the symbol in read-only section.
The total size decreases :)
Ah, yes. The state of my local build tree is not good to calculate it...
In fact, the relocatable object decreases its size when built with a
header package from Ubuntu repository.
Thanks
Takashi Sakamoto
Re: [PATCH v3 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20
_IEC958_SUBFRAME
SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE
+#defineSNDRV_PCM_FORMAT_S20SNDRV_PCM_FORMAT_S20_LE
+#defineSNDRV_PCM_FORMAT_U20SNDRV_PCM_FORMAT_U20_LE
#endif
#ifdef SNDRV_BIG_ENDIAN
#define SNDRV_PCM_FORMAT_S16SNDRV_PCM_FORMAT_S16_BE
@@ -259,6 +266,8 @@ typedef int __bitwise snd_pcm_format_t;
#define SNDRV_PCM_FORMAT_FLOAT SNDRV_PCM_FORMAT_FLOAT_BE
#define SNDRV_PCM_FORMAT_FLOAT64SNDRV_PCM_FORMAT_FLOAT64_BE
#define SNDRV_PCM_FORMAT_IEC958_SUBFRAME
SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE
+#defineSNDRV_PCM_FORMAT_S20SNDRV_PCM_FORMAT_S20_BE
+#defineSNDRV_PCM_FORMAT_U20SNDRV_PCM_FORMAT_U20_BE
#endif
typedef int __bitwise snd_pcm_subformat_t;
diff --git a/sound/core/pcm_misc.c b/sound/core/pcm_misc.c
index 9be81025372f..c4eb561d2008 100644
--- a/sound/core/pcm_misc.c
+++ b/sound/core/pcm_misc.c
@@ -163,13 +163,30 @@ static struct pcm_format_data
pcm_formats[(INT)SNDRV_PCM_FORMAT_LAST+1] = {
.width = 32, .phys = 32, .le = 0, .signd = 0,
.silence = { 0x69, 0x69, 0x69, 0x69 },
},
- /* FIXME: the following three formats are not defined properly yet */
+ /* FIXME: the following two formats are not defined properly yet */
[SNDRV_PCM_FORMAT_MPEG] = {
.le = -1, .signd = -1,
},
[SNDRV_PCM_FORMAT_GSM] = {
.le = -1, .signd = -1,
},
+ [SNDRV_PCM_FORMAT_S20_LE] = {
+ .width = 20, .phys = 32, .le = 1, .signd = 1,
+ .silence = {},
+ },
+ [SNDRV_PCM_FORMAT_S20_BE] = {
+ .width = 20, .phys = 32, .le = 0, .signd = 1,
+ .silence = {},
+ },
+ [SNDRV_PCM_FORMAT_U20_LE] = {
+ .width = 20, .phys = 32, .le = 1, .signd = 0,
+ .silence = { 0x00, 0x00, 0x08, 0x00 },
+ },
+ [SNDRV_PCM_FORMAT_U20_BE] = {
+ .width = 20, .phys = 32, .le = 0, .signd = 0,
+ .silence = { 0x00, 0x08, 0x00, 0x00 },
+ },
+ /* FIXME: the following format is not defined properly yet */
[SNDRV_PCM_FORMAT_SPECIAL] = {
.le = -1, .signd = -1,
},
Looks good to me.
Reviewed-by: Takashi Sakamoto <o-taka...@sakamocchi.jp>
In next time to post any of your v2 patchset, it's better to add
commenters of v1 patchset to CC list, so that your patch reaches the
person who has practical interests in it.
I'm waiting for your another patch to alsa-lib.
Thanks
Takashi Sakamoto
Re: [PATCH v3 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20
UBFRAME_LE
+#defineSNDRV_PCM_FORMAT_S20SNDRV_PCM_FORMAT_S20_LE
+#defineSNDRV_PCM_FORMAT_U20SNDRV_PCM_FORMAT_U20_LE
#endif
#ifdef SNDRV_BIG_ENDIAN
#define SNDRV_PCM_FORMAT_S16SNDRV_PCM_FORMAT_S16_BE
@@ -259,6 +266,8 @@ typedef int __bitwise snd_pcm_format_t;
#define SNDRV_PCM_FORMAT_FLOAT SNDRV_PCM_FORMAT_FLOAT_BE
#define SNDRV_PCM_FORMAT_FLOAT64SNDRV_PCM_FORMAT_FLOAT64_BE
#define SNDRV_PCM_FORMAT_IEC958_SUBFRAME
SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE
+#defineSNDRV_PCM_FORMAT_S20SNDRV_PCM_FORMAT_S20_BE
+#defineSNDRV_PCM_FORMAT_U20SNDRV_PCM_FORMAT_U20_BE
#endif
typedef int __bitwise snd_pcm_subformat_t;
diff --git a/sound/core/pcm_misc.c b/sound/core/pcm_misc.c
index 9be81025372f..c4eb561d2008 100644
--- a/sound/core/pcm_misc.c
+++ b/sound/core/pcm_misc.c
@@ -163,13 +163,30 @@ static struct pcm_format_data
pcm_formats[(INT)SNDRV_PCM_FORMAT_LAST+1] = {
.width = 32, .phys = 32, .le = 0, .signd = 0,
.silence = { 0x69, 0x69, 0x69, 0x69 },
},
- /* FIXME: the following three formats are not defined properly yet */
+ /* FIXME: the following two formats are not defined properly yet */
[SNDRV_PCM_FORMAT_MPEG] = {
.le = -1, .signd = -1,
},
[SNDRV_PCM_FORMAT_GSM] = {
.le = -1, .signd = -1,
},
+ [SNDRV_PCM_FORMAT_S20_LE] = {
+ .width = 20, .phys = 32, .le = 1, .signd = 1,
+ .silence = {},
+ },
+ [SNDRV_PCM_FORMAT_S20_BE] = {
+ .width = 20, .phys = 32, .le = 0, .signd = 1,
+ .silence = {},
+ },
+ [SNDRV_PCM_FORMAT_U20_LE] = {
+ .width = 20, .phys = 32, .le = 1, .signd = 0,
+ .silence = { 0x00, 0x00, 0x08, 0x00 },
+ },
+ [SNDRV_PCM_FORMAT_U20_BE] = {
+ .width = 20, .phys = 32, .le = 0, .signd = 0,
+ .silence = { 0x00, 0x08, 0x00, 0x00 },
+ },
+ /* FIXME: the following format is not defined properly yet */
[SNDRV_PCM_FORMAT_SPECIAL] = {
.le = -1, .signd = -1,
},
Looks good to me.
Reviewed-by: Takashi Sakamoto
In next time to post any of your v2 patchset, it's better to add
commenters of v1 patchset to CC list, so that your patch reaches the
person who has practical interests in it.
I'm waiting for your another patch to alsa-lib.
Thanks
Takashi Sakamoto
Re: [PATCH] ALSA: drivers: make array 'names' const, reduces object code size
Hi,
On Nov 27 2017 21:58, Colin King wrote:
From: Colin Ian King <colin.k...@canonical.com>
Don't populate array 'names' on the stack but instead make them static.
Makes the object code smaller by 50 bytes:
Before:
text data bss dec hex filename
21237 91921120 315497b3d linux/sound/drivers/dummy.o
After:
text data bss dec hex filename
21095 92801120 314957b07 linux/sound/drivers/dummy.o
(gcc version 7.2.0 x86_64)
Signed-off-by: Colin Ian King <colin.k...@canonical.com>
---
sound/drivers/dummy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index 7b2b1f766b00..69db45bc0197 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -830,7 +830,7 @@ static int snd_dummy_capsrc_put(struct snd_kcontrol
*kcontrol, struct snd_ctl_el
static int snd_dummy_iobox_info(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_info *info)
{
- const char *const names[] = { "None", "CD Player" };
+ static const char *const names[] = { "None", "CD Player" };
return snd_ctl_enum_info(info, 1, 2, names);
}
Total size of snd-dummy.ko increases but this patch has an advantage to
have the symbol in read-only section. This looks good to me.
Reviewed-by: Takashi Sakamoto <o-taka...@sakamocchi.jp>
Another issue is addressed by the others, but here I focus on the
original intention of this patch.
Thanks
Takashi Sakamoto
Re: [PATCH] ALSA: drivers: make array 'names' const, reduces object code size
Hi,
On Nov 27 2017 21:58, Colin King wrote:
From: Colin Ian King
Don't populate array 'names' on the stack but instead make them static.
Makes the object code smaller by 50 bytes:
Before:
text data bss dec hex filename
21237 91921120 315497b3d linux/sound/drivers/dummy.o
After:
text data bss dec hex filename
21095 92801120 314957b07 linux/sound/drivers/dummy.o
(gcc version 7.2.0 x86_64)
Signed-off-by: Colin Ian King
---
sound/drivers/dummy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index 7b2b1f766b00..69db45bc0197 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -830,7 +830,7 @@ static int snd_dummy_capsrc_put(struct snd_kcontrol
*kcontrol, struct snd_ctl_el
static int snd_dummy_iobox_info(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_info *info)
{
- const char *const names[] = { "None", "CD Player" };
+ static const char *const names[] = { "None", "CD Player" };
return snd_ctl_enum_info(info, 1, 2, names);
}
Total size of snd-dummy.ko increases but this patch has an advantage to
have the symbol in read-only section. This looks good to me.
Reviewed-by: Takashi Sakamoto
Another issue is addressed by the others, but here I focus on the
original intention of this patch.
Thanks
Takashi Sakamoto
Re: [PATCH v2 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20
Hi,
On Nov 24 2017 08:31, Maciej S. Szmigiero wrote:
This format is similar to existing SNDRV_PCM_FORMAT_{S,U}20_3 that keep
20-bit PCM samples in 3 bytes, however i.MX6 platform SSI FIFO does not
allow 3-byte accesses (including DMA) so a 4-byte (more conventional)
format is needed for it.
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
Changes from v1: Drop "_4" suffix from these formats since they aren't
non-standard ones, use empty format slots starting from format number 25
for them, add information that they are LSB justified formats.
Corresponding alsa-lib changes will be posted as soon as this patch is
merged on the kernel side, to keep alsa-lib and kernel synchronized.
include/sound/pcm.h | 8
include/sound/soc-dai.h | 2 ++
include/uapi/sound/asound.h | 9 +
sound/core/pcm_misc.c | 16
4 files changed, 35 insertions(+)
...
diff --git a/sound/core/pcm_misc.c b/sound/core/pcm_misc.c
index 9be81025372f..c62bfe27106f 100644
--- a/sound/core/pcm_misc.c
+++ b/sound/core/pcm_misc.c
@@ -170,6 +170,22 @@ static struct pcm_format_data
pcm_formats[(INT)SNDRV_PCM_FORMAT_LAST+1] = {
[SNDRV_PCM_FORMAT_GSM] = {
.le = -1, .signd = -1,
},
+ [SNDRV_PCM_FORMAT_S20_LE] = {
+ .width = 20, .phys = 32, .le = 1, .signd = 1,
+ .silence = {},
+ },
+ [SNDRV_PCM_FORMAT_S20_BE] = {
+ .width = 20, .phys = 32, .le = 0, .signd = 1,
+ .silence = {},
+ },
+ [SNDRV_PCM_FORMAT_U20_LE] = {
+ .width = 20, .phys = 32, .le = 1, .signd = 0,
+ .silence = { 0x00, 0x00, 0x08, 0x00 },
+ },
+ [SNDRV_PCM_FORMAT_U20_BE] = {
+ .width = 20, .phys = 32, .le = 0, .signd = 0,
+ .silence = { 0x00, 0x08, 0x00, 0x00 },
+ },
[SNDRV_PCM_FORMAT_SPECIAL] = {
.le = -1, .signd = -1,
},
Before applying this patch:
166 /* FIXME: the following three formats are not defined
properly yet */
167 [SNDRV_PCM_FORMAT_MPEG] = {
168 .le = -1, .signd = -1,
169 },
170 [SNDRV_PCM_FORMAT_GSM] = {
171 .le = -1, .signd = -1,
172 },
173 [SNDRV_PCM_FORMAT_SPECIAL] = {
174 .le = -1, .signd = -1,
175 },
After applying this patch:
166 /* FIXME: the following three formats are not defined
properly yet */
167 [SNDRV_PCM_FORMAT_MPEG] = {
168 .le = -1, .signd = -1,
169 },
170 [SNDRV_PCM_FORMAT_GSM] = {
171 .le = -1, .signd = -1,
172 },
173 [SNDRV_PCM_FORMAT_S20_LE] = {
174 .width = 20, .phys = 32, .le = 1, .signd = 1,
175 .silence = {},
176 },
177 [SNDRV_PCM_FORMAT_S20_BE] = {
178 .width = 20, .phys = 32, .le = 0, .signd = 1,
179 .silence = {},
180 },
181 [SNDRV_PCM_FORMAT_U20_LE] = {
182 .width = 20, .phys = 32, .le = 1, .signd = 0,
183 .silence = { 0x00, 0x00, 0x08, 0x00 },
184 },
185 [SNDRV_PCM_FORMAT_U20_BE] = {
186 .width = 20, .phys = 32, .le = 0, .signd = 0,
187 .silence = { 0x00, 0x08, 0x00, 0x00 },
188 },
189 [SNDRV_PCM_FORMAT_SPECIAL] = {
190 .le = -1, .signd = -1,
191 },
I think it good to add an alternative comment for each of entry which is
not defined yet, like:
-> 166 /* FIXME: this format is not defined properly yet */
167 [SNDRV_PCM_FORMAT_MPEG] = {
168 .le = -1, .signd = -1,
169 },
-> 170 /* FIXME: this format is not defined properly yet */
171 [SNDRV_PCM_FORMAT_GSM] = {
172 .le = -1, .signd = -1,
173 },
174 [SNDRV_PCM_FORMAT_S20_LE] = {
175 .width = 20, .phys = 32, .le = 1, .signd = 1,
176 .silence = {},
177 },
178 [SNDRV_PCM_FORMAT_S20_BE] = {
179 .width = 20, .phys = 32, .le = 0, .signd = 1,
180 .silence = {},
181 },
182 [SNDRV_PCM_FORMAT_U20_LE] = {
183 .width = 20, .phys = 32, .le = 1, .signd = 0,
184 .silence = { 0x00, 0x00, 0x08, 0x00 },
185 },
186 [SNDRV_PCM_FORMAT_U20_BE] = {
187 .width = 20, .phys = 32, .le = 0, .signd = 0,
188 .silence = { 0x00, 0x08, 0x00, 0x00 },
189 },
-> 190 /* FIXME: this format is not defined properly yet */
191 [SNDRV_PCM_FORMAT_SPECIAL] = {
192 .le = -1, .signd = -1,
193 },
Regards
Takashi Sakamoto
Re: [PATCH v2 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20
Hi,
On Nov 24 2017 08:31, Maciej S. Szmigiero wrote:
This format is similar to existing SNDRV_PCM_FORMAT_{S,U}20_3 that keep
20-bit PCM samples in 3 bytes, however i.MX6 platform SSI FIFO does not
allow 3-byte accesses (including DMA) so a 4-byte (more conventional)
format is needed for it.
Signed-off-by: Maciej S. Szmigiero
---
Changes from v1: Drop "_4" suffix from these formats since they aren't
non-standard ones, use empty format slots starting from format number 25
for them, add information that they are LSB justified formats.
Corresponding alsa-lib changes will be posted as soon as this patch is
merged on the kernel side, to keep alsa-lib and kernel synchronized.
include/sound/pcm.h | 8
include/sound/soc-dai.h | 2 ++
include/uapi/sound/asound.h | 9 +
sound/core/pcm_misc.c | 16
4 files changed, 35 insertions(+)
...
diff --git a/sound/core/pcm_misc.c b/sound/core/pcm_misc.c
index 9be81025372f..c62bfe27106f 100644
--- a/sound/core/pcm_misc.c
+++ b/sound/core/pcm_misc.c
@@ -170,6 +170,22 @@ static struct pcm_format_data
pcm_formats[(INT)SNDRV_PCM_FORMAT_LAST+1] = {
[SNDRV_PCM_FORMAT_GSM] = {
.le = -1, .signd = -1,
},
+ [SNDRV_PCM_FORMAT_S20_LE] = {
+ .width = 20, .phys = 32, .le = 1, .signd = 1,
+ .silence = {},
+ },
+ [SNDRV_PCM_FORMAT_S20_BE] = {
+ .width = 20, .phys = 32, .le = 0, .signd = 1,
+ .silence = {},
+ },
+ [SNDRV_PCM_FORMAT_U20_LE] = {
+ .width = 20, .phys = 32, .le = 1, .signd = 0,
+ .silence = { 0x00, 0x00, 0x08, 0x00 },
+ },
+ [SNDRV_PCM_FORMAT_U20_BE] = {
+ .width = 20, .phys = 32, .le = 0, .signd = 0,
+ .silence = { 0x00, 0x08, 0x00, 0x00 },
+ },
[SNDRV_PCM_FORMAT_SPECIAL] = {
.le = -1, .signd = -1,
},
Before applying this patch:
166 /* FIXME: the following three formats are not defined
properly yet */
167 [SNDRV_PCM_FORMAT_MPEG] = {
168 .le = -1, .signd = -1,
169 },
170 [SNDRV_PCM_FORMAT_GSM] = {
171 .le = -1, .signd = -1,
172 },
173 [SNDRV_PCM_FORMAT_SPECIAL] = {
174 .le = -1, .signd = -1,
175 },
After applying this patch:
166 /* FIXME: the following three formats are not defined
properly yet */
167 [SNDRV_PCM_FORMAT_MPEG] = {
168 .le = -1, .signd = -1,
169 },
170 [SNDRV_PCM_FORMAT_GSM] = {
171 .le = -1, .signd = -1,
172 },
173 [SNDRV_PCM_FORMAT_S20_LE] = {
174 .width = 20, .phys = 32, .le = 1, .signd = 1,
175 .silence = {},
176 },
177 [SNDRV_PCM_FORMAT_S20_BE] = {
178 .width = 20, .phys = 32, .le = 0, .signd = 1,
179 .silence = {},
180 },
181 [SNDRV_PCM_FORMAT_U20_LE] = {
182 .width = 20, .phys = 32, .le = 1, .signd = 0,
183 .silence = { 0x00, 0x00, 0x08, 0x00 },
184 },
185 [SNDRV_PCM_FORMAT_U20_BE] = {
186 .width = 20, .phys = 32, .le = 0, .signd = 0,
187 .silence = { 0x00, 0x08, 0x00, 0x00 },
188 },
189 [SNDRV_PCM_FORMAT_SPECIAL] = {
190 .le = -1, .signd = -1,
191 },
I think it good to add an alternative comment for each of entry which is
not defined yet, like:
-> 166 /* FIXME: this format is not defined properly yet */
167 [SNDRV_PCM_FORMAT_MPEG] = {
168 .le = -1, .signd = -1,
169 },
-> 170 /* FIXME: this format is not defined properly yet */
171 [SNDRV_PCM_FORMAT_GSM] = {
172 .le = -1, .signd = -1,
173 },
174 [SNDRV_PCM_FORMAT_S20_LE] = {
175 .width = 20, .phys = 32, .le = 1, .signd = 1,
176 .silence = {},
177 },
178 [SNDRV_PCM_FORMAT_S20_BE] = {
179 .width = 20, .phys = 32, .le = 0, .signd = 1,
180 .silence = {},
181 },
182 [SNDRV_PCM_FORMAT_U20_LE] = {
183 .width = 20, .phys = 32, .le = 1, .signd = 0,
184 .silence = { 0x00, 0x00, 0x08, 0x00 },
185 },
186 [SNDRV_PCM_FORMAT_U20_BE] = {
187 .width = 20, .phys = 32, .le = 0, .signd = 0,
188 .silence = { 0x00, 0x08, 0x00, 0x00 },
189 },
-> 190 /* FIXME: this format is not defined properly yet */
191 [SNDRV_PCM_FORMAT_SPECIAL] = {
192 .le = -1, .signd = -1,
193 },
Regards
Takashi Sakamoto
Re: [PATCH 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20_4
On Nov 23 2017 08:44, Maciej S. Szmigiero wrote:
On 23.11.2017 00:27, Takashi Sakamoto wrote:
On Nov 23 2017 04:17, Maciej S. Szmigiero wrote:
(..)
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -236,7 +236,11 @@ typedef int __bitwise snd_pcm_format_t;
#define SNDRV_PCM_FORMAT_DSD_U32_LE ((__force snd_pcm_format_t) 50) /*
DSD, 4-byte samples DSD (x32), little endian */
#define SNDRV_PCM_FORMAT_DSD_U16_BE ((__force snd_pcm_format_t) 51) /*
DSD, 2-byte samples DSD (x16), big endian */
#define SNDRV_PCM_FORMAT_DSD_U32_BE ((__force snd_pcm_format_t) 52) /*
DSD, 4-byte samples DSD (x32), big endian */
-#define SNDRV_PCM_FORMAT_LAST SNDRV_PCM_FORMAT_DSD_U32_BE
+#define SNDRV_PCM_FORMAT_S20_4LE ((__force snd_pcm_format_t) 53) /*
in four bytes */
+#define SNDRV_PCM_FORMAT_S20_4BE ((__force snd_pcm_format_t) 54) /*
in four bytes */
+#define SNDRV_PCM_FORMAT_U20_4LE ((__force snd_pcm_format_t) 55) /*
in four bytes */
+#define SNDRV_PCM_FORMAT_U20_4BE ((__force snd_pcm_format_t) 56) /*
in four bytes */
+#define SNDRV_PCM_FORMAT_LAST SNDRV_PCM_FORMAT_U20_4BE
In my opinion, for this type of definition, it's better to declare
left/right-adjusted or padding side. (Of course, silence definition is already
a hint, however the lack of information forces developers to have a careful
behaviour to handle entries on the list.
(I note that in current ALSA PCM interface there's no way to deliver
MSB/LSB-first information about sample format.)
No other sound format includes this information in its name
You overlook comments in 'SNDRV_PCM_FORMAT_[U|S]24_[LE|BE]'. Let me
refer to them [1]:
198 #define SNDRV_PCM_FORMAT_S24_LE ((__force snd_pcm_format_t) 6) /*
low three bytes */
199 #define SNDRV_PCM_FORMAT_S24_BE ((__force snd_pcm_format_t) 7) /*
low three bytes */
200 #define SNDRV_PCM_FORMAT_U24_LE ((__force snd_pcm_format_t) 8) /*
low three bytes */
201 #define SNDRV_PCM_FORMAT_U24_BE ((__force snd_pcm_format_t) 9) /*
low three bytes */
In your way, these types of format can be represented by
'SNDRV_PCM_FORMAT_[U|S]24_4[LE|BE]', thus for playback direction they
mean:
```
#include
#include
uint32_t *buf;
uint32_t sample;
snd_pcm_format_t format;
sample = generate_a_sample();
(sample & ~0x00ff) /* invalid bits as sample */
if (format == SNDRV_PCM_FORMAT_[U|S]24_LE) {
buf[0] = htole32(sample);
else
buf[0] = htobe32(sample);
/* transfer content of the buf via ALSA kernel stuffs. */
```
The comments are good enough for application developers in an aspect of
a position for padding.
In general, studying from the past is preferable behaviour to be genius,
however accumulated history includes mistakes and defects. Just
pretending the past is not so genius, without further consideration.
Actually additions of the rest of entries for PCM format were done
without enough cares of what information they give to application
developers. Adding new entries is easier than fixing and improving them
once exposed. It's a reason that they're left what they're.
I wish you had enough care to assist applications developers. Without
applications, drivers are worthless and just waste of code base.
so if we name
these formats SNDRV_PCM_FORMAT_{S, U}20LSB_4 they are going to have it
inconsistent with every other one
(I assume you meant to include such information in a format name?).
But information about whether this format is MSB or LSB justified can be
added in a comment so the situation is clear for other developers from
the definition without needing to read the actual processing code.
For consistency of the other entries, this is not so preferable, in my
opinion. So I didn't suggest it and just noted.
Additionally, alsa-lib includes some codes related to the definition[1]. If
you'd like to thing goes well out of ALSA SoC part, it's better to submit
changes to the library as well.
[1]
http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm_misc.c;h=5420b1895713a3aec3624a5218794a7b49baf167;hb=HEAD
I have alsa-lib changes ready for these formats - they were needed to
test these patches, will post them when this is merged on the kernel
side (in case some changes are needed which affect both).
Please pay enough care when writing patch comment. Silence means
nothing, at least for reviewers, even if you have good preparations.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h?h=sound-4.15-rc1#n198
Regards
Takashi Sakamoto
Re: [PATCH 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20_4
On Nov 23 2017 08:44, Maciej S. Szmigiero wrote:
On 23.11.2017 00:27, Takashi Sakamoto wrote:
On Nov 23 2017 04:17, Maciej S. Szmigiero wrote:
(..)
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -236,7 +236,11 @@ typedef int __bitwise snd_pcm_format_t;
#define SNDRV_PCM_FORMAT_DSD_U32_LE ((__force snd_pcm_format_t) 50) /*
DSD, 4-byte samples DSD (x32), little endian */
#define SNDRV_PCM_FORMAT_DSD_U16_BE ((__force snd_pcm_format_t) 51) /*
DSD, 2-byte samples DSD (x16), big endian */
#define SNDRV_PCM_FORMAT_DSD_U32_BE ((__force snd_pcm_format_t) 52) /*
DSD, 4-byte samples DSD (x32), big endian */
-#define SNDRV_PCM_FORMAT_LAST SNDRV_PCM_FORMAT_DSD_U32_BE
+#define SNDRV_PCM_FORMAT_S20_4LE ((__force snd_pcm_format_t) 53) /*
in four bytes */
+#define SNDRV_PCM_FORMAT_S20_4BE ((__force snd_pcm_format_t) 54) /*
in four bytes */
+#define SNDRV_PCM_FORMAT_U20_4LE ((__force snd_pcm_format_t) 55) /*
in four bytes */
+#define SNDRV_PCM_FORMAT_U20_4BE ((__force snd_pcm_format_t) 56) /*
in four bytes */
+#define SNDRV_PCM_FORMAT_LAST SNDRV_PCM_FORMAT_U20_4BE
In my opinion, for this type of definition, it's better to declare
left/right-adjusted or padding side. (Of course, silence definition is already
a hint, however the lack of information forces developers to have a careful
behaviour to handle entries on the list.
(I note that in current ALSA PCM interface there's no way to deliver
MSB/LSB-first information about sample format.)
No other sound format includes this information in its name
You overlook comments in 'SNDRV_PCM_FORMAT_[U|S]24_[LE|BE]'. Let me
refer to them [1]:
198 #define SNDRV_PCM_FORMAT_S24_LE ((__force snd_pcm_format_t) 6) /*
low three bytes */
199 #define SNDRV_PCM_FORMAT_S24_BE ((__force snd_pcm_format_t) 7) /*
low three bytes */
200 #define SNDRV_PCM_FORMAT_U24_LE ((__force snd_pcm_format_t) 8) /*
low three bytes */
201 #define SNDRV_PCM_FORMAT_U24_BE ((__force snd_pcm_format_t) 9) /*
low three bytes */
In your way, these types of format can be represented by
'SNDRV_PCM_FORMAT_[U|S]24_4[LE|BE]', thus for playback direction they
mean:
```
#include
#include
uint32_t *buf;
uint32_t sample;
snd_pcm_format_t format;
sample = generate_a_sample();
(sample & ~0x00ff) /* invalid bits as sample */
if (format == SNDRV_PCM_FORMAT_[U|S]24_LE) {
buf[0] = htole32(sample);
else
buf[0] = htobe32(sample);
/* transfer content of the buf via ALSA kernel stuffs. */
```
The comments are good enough for application developers in an aspect of
a position for padding.
In general, studying from the past is preferable behaviour to be genius,
however accumulated history includes mistakes and defects. Just
pretending the past is not so genius, without further consideration.
Actually additions of the rest of entries for PCM format were done
without enough cares of what information they give to application
developers. Adding new entries is easier than fixing and improving them
once exposed. It's a reason that they're left what they're.
I wish you had enough care to assist applications developers. Without
applications, drivers are worthless and just waste of code base.
so if we name
these formats SNDRV_PCM_FORMAT_{S, U}20LSB_4 they are going to have it
inconsistent with every other one
(I assume you meant to include such information in a format name?).
But information about whether this format is MSB or LSB justified can be
added in a comment so the situation is clear for other developers from
the definition without needing to read the actual processing code.
For consistency of the other entries, this is not so preferable, in my
opinion. So I didn't suggest it and just noted.
Additionally, alsa-lib includes some codes related to the definition[1]. If
you'd like to thing goes well out of ALSA SoC part, it's better to submit
changes to the library as well.
[1]
http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm_misc.c;h=5420b1895713a3aec3624a5218794a7b49baf167;hb=HEAD
I have alsa-lib changes ready for these formats - they were needed to
test these patches, will post them when this is merged on the kernel
side (in case some changes are needed which affect both).
Please pay enough care when writing patch comment. Silence means
nothing, at least for reviewers, even if you have good preparations.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h?h=sound-4.15-rc1#n198
Regards
Takashi Sakamoto
Re: [PATCH 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20_4
On Nov 23 2017 04:17, Maciej S. Szmigiero wrote:
This format is similar to existing SNDRV_PCM_FORMAT_{S,U}20_3 that keep
20-bit PCM samples in 3 bytes, however i.MX6 platform SSI FIFO does not
allow 3-byte accesses (including DMA) so a 4-byte format is needed for it.
Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
include/sound/pcm.h | 8
include/sound/soc-dai.h | 2 ++
include/uapi/sound/asound.h | 10 +-
sound/core/pcm_misc.c | 16
4 files changed, 35 insertions(+), 1 deletion(-)
... >
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index c227ccba60ae..69b661816491 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -236,7 +236,11 @@ typedef int __bitwise snd_pcm_format_t;
#define SNDRV_PCM_FORMAT_DSD_U32_LE ((__force snd_pcm_format_t) 50)
/* DSD, 4-byte samples DSD (x32), little endian */
#define SNDRV_PCM_FORMAT_DSD_U16_BE ((__force snd_pcm_format_t) 51)
/* DSD, 2-byte samples DSD (x16), big endian */
#define SNDRV_PCM_FORMAT_DSD_U32_BE ((__force snd_pcm_format_t) 52)
/* DSD, 4-byte samples DSD (x32), big endian */
-#defineSNDRV_PCM_FORMAT_LAST SNDRV_PCM_FORMAT_DSD_U32_BE
+#defineSNDRV_PCM_FORMAT_S20_4LE((__force snd_pcm_format_t) 53)
/* in four bytes */
+#defineSNDRV_PCM_FORMAT_S20_4BE((__force snd_pcm_format_t) 54)
/* in four bytes */
+#defineSNDRV_PCM_FORMAT_U20_4LE((__force snd_pcm_format_t) 55)
/* in four bytes */
+#defineSNDRV_PCM_FORMAT_U20_4BE((__force snd_pcm_format_t) 56)
/* in four bytes */
+#defineSNDRV_PCM_FORMAT_LAST SNDRV_PCM_FORMAT_U20_4BE
In my opinion, for this type of definition, it's better to declare
left/right-adjusted or padding side. (Of course, silence definition is
already a hint, however the lack of information forces developers to
have a careful behaviour to handle entries on the list.)
(I note that in current ALSA PCM interface there's no way to deliver
MSB/LSB-first information about sample format.)
Additionally, alsa-lib includes some codes related to the definition[1].
If you'd like to thing goes well out of ALSA SoC part, it's better to
submit changes to the library as well.
[1]
http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm_misc.c;h=5420b1895713a3aec3624a5218794a7b49baf167;hb=HEAD
Regards
Takashi Sakamoto
Re: [PATCH 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20_4
On Nov 23 2017 04:17, Maciej S. Szmigiero wrote:
This format is similar to existing SNDRV_PCM_FORMAT_{S,U}20_3 that keep
20-bit PCM samples in 3 bytes, however i.MX6 platform SSI FIFO does not
allow 3-byte accesses (including DMA) so a 4-byte format is needed for it.
Signed-off-by: Maciej S. Szmigiero
---
include/sound/pcm.h | 8
include/sound/soc-dai.h | 2 ++
include/uapi/sound/asound.h | 10 +-
sound/core/pcm_misc.c | 16
4 files changed, 35 insertions(+), 1 deletion(-)
... >
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index c227ccba60ae..69b661816491 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -236,7 +236,11 @@ typedef int __bitwise snd_pcm_format_t;
#define SNDRV_PCM_FORMAT_DSD_U32_LE ((__force snd_pcm_format_t) 50)
/* DSD, 4-byte samples DSD (x32), little endian */
#define SNDRV_PCM_FORMAT_DSD_U16_BE ((__force snd_pcm_format_t) 51)
/* DSD, 2-byte samples DSD (x16), big endian */
#define SNDRV_PCM_FORMAT_DSD_U32_BE ((__force snd_pcm_format_t) 52)
/* DSD, 4-byte samples DSD (x32), big endian */
-#defineSNDRV_PCM_FORMAT_LAST SNDRV_PCM_FORMAT_DSD_U32_BE
+#defineSNDRV_PCM_FORMAT_S20_4LE((__force snd_pcm_format_t) 53)
/* in four bytes */
+#defineSNDRV_PCM_FORMAT_S20_4BE((__force snd_pcm_format_t) 54)
/* in four bytes */
+#defineSNDRV_PCM_FORMAT_U20_4LE((__force snd_pcm_format_t) 55)
/* in four bytes */
+#defineSNDRV_PCM_FORMAT_U20_4BE((__force snd_pcm_format_t) 56)
/* in four bytes */
+#defineSNDRV_PCM_FORMAT_LAST SNDRV_PCM_FORMAT_U20_4BE
In my opinion, for this type of definition, it's better to declare
left/right-adjusted or padding side. (Of course, silence definition is
already a hint, however the lack of information forces developers to
have a careful behaviour to handle entries on the list.)
(I note that in current ALSA PCM interface there's no way to deliver
MSB/LSB-first information about sample format.)
Additionally, alsa-lib includes some codes related to the definition[1].
If you'd like to thing goes well out of ALSA SoC part, it's better to
submit changes to the library as well.
[1]
http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm_misc.c;h=5420b1895713a3aec3624a5218794a7b49baf167;hb=HEAD
Regards
Takashi Sakamoto
Re: [RFC PATCH v2 5/7] uapi: sound: Avoid using timespec for struct snd_ctl_elem_value
Hi,
On Nov 2 2017 20:06, Baolin Wang wrote:
The struct snd_ctl_elem_value will use 'timespec' type variables to record
timestamp, which is not year 2038 safe on 32bits system.
Since there are no drivers will implemented the tstamp member of the
struct snd_ctl_elem_value, and also the stucture size will not be changed
if we change timespec to s64 for tstamp member of struct snd_ctl_elem_value.
Thus we can simply change timespec to s64 for tstamp member to avoid
using the type which is not year 2038 safe on 32bits system.
Signed-off-by: Baolin Wang <baolin.w...@linaro.org>
---
include/uapi/sound/asound.h |8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 1949923..fabb283 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -943,8 +943,12 @@ struct snd_ctl_elem_value {
} bytes;
struct snd_aes_iec958 iec958;
} value;/* RO */
- struct timespec tstamp;
- unsigned char reserved[128-sizeof(struct timespec)];
+#ifndef __KERNEL__
+ struct { s64 tv_sec; s64 tv_nsec; } tstamp;
+ unsigned char reserved[128-sizeof(struct { s64 tv_sec; s64 tv_nsec; })];
+#else
+ unsigned char reserved[128];
+#endif
};
struct snd_ctl_tlv {
As long as I know, via any APIs in alsa-lib[1], 'struct
snd_ctl_elem_value.tstamp' is not available, fortunately.
In the library, applications are not expected to access to this
structure directly. The applications get opaque pointer to the structure
and must use any control APIs to operate it. Actually the library
produce no API to handle 'struct snd_ctl_elem_value.tstamp'. This means
that we can drop this member from alsa-lib without decline of functionality.
As you know, the member is abandoned in kernel side as well. This allows
us to judge that this feature is not practically used by any userspace
implementations such as tinyalsa[2].
In my opinion, we can have a plan to drop this useless member instead of
this patch. Of course, we should have enough investigation and
consideration about its meaning on ALSA control interface in advance of
actual removal.
[1] http://git.alsa-project.org/?p=alsa-lib.git
[2] https://android.googlesource.com/platform/external/tinyalsa/
Thanks
Takashi Sakamoto
Re: [RFC PATCH v2 5/7] uapi: sound: Avoid using timespec for struct snd_ctl_elem_value
Hi,
On Nov 2 2017 20:06, Baolin Wang wrote:
The struct snd_ctl_elem_value will use 'timespec' type variables to record
timestamp, which is not year 2038 safe on 32bits system.
Since there are no drivers will implemented the tstamp member of the
struct snd_ctl_elem_value, and also the stucture size will not be changed
if we change timespec to s64 for tstamp member of struct snd_ctl_elem_value.
Thus we can simply change timespec to s64 for tstamp member to avoid
using the type which is not year 2038 safe on 32bits system.
Signed-off-by: Baolin Wang
---
include/uapi/sound/asound.h |8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 1949923..fabb283 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -943,8 +943,12 @@ struct snd_ctl_elem_value {
} bytes;
struct snd_aes_iec958 iec958;
} value;/* RO */
- struct timespec tstamp;
- unsigned char reserved[128-sizeof(struct timespec)];
+#ifndef __KERNEL__
+ struct { s64 tv_sec; s64 tv_nsec; } tstamp;
+ unsigned char reserved[128-sizeof(struct { s64 tv_sec; s64 tv_nsec; })];
+#else
+ unsigned char reserved[128];
+#endif
};
struct snd_ctl_tlv {
As long as I know, via any APIs in alsa-lib[1], 'struct
snd_ctl_elem_value.tstamp' is not available, fortunately.
In the library, applications are not expected to access to this
structure directly. The applications get opaque pointer to the structure
and must use any control APIs to operate it. Actually the library
produce no API to handle 'struct snd_ctl_elem_value.tstamp'. This means
that we can drop this member from alsa-lib without decline of functionality.
As you know, the member is abandoned in kernel side as well. This allows
us to judge that this feature is not practically used by any userspace
implementations such as tinyalsa[2].
In my opinion, we can have a plan to drop this useless member instead of
this patch. Of course, we should have enough investigation and
consideration about its meaning on ALSA control interface in advance of
actual removal.
[1] http://git.alsa-project.org/?p=alsa-lib.git
[2] https://android.googlesource.com/platform/external/tinyalsa/
Thanks
Takashi Sakamoto
Re: [RFC] ALSA: vsnd: Add Xen para-virtualized frontend driver
On Oct 30 2017 15:33, Oleksandr Andrushchenko wrote: This is an attempt to summarize previous discussions on Xen para-virtual sound driver. A first attempt has been made to upstream the driver [1] which brought number of fundamental questions, one of the biggest ones was that the frontend driver has no means to synchronize its period elapsed event with the host driver, but uses software emulation on the guest side [2] with a timer. In order to address this a change to the existing Xen para-virtual sound protocol [3] was proposed to fill this gap [4] and remove emulation: 1. Introduced a new event channel from back to front 2. New event with number of bytes played/captured (XENSND_EVT_CUR_POS, to be used for sending snd_pcm_period_elapsed at frontend (in Linux implementation, sent in bytes, not frames to make the protocol generic and consistent) 3. New request for playback/capture control (XENSND_OP_TRIGGER) with start/pause/stop/resume sub-ops. Along with these changes other comments on the driver were addressed, e.g. split into smaller chunks, moved the driver from misc to xen etc. [5]. Hope, this helps to get the full picture of what was discussed and makes it possible to move forward: if the approach seems ok, then I'll start upstreaming the changes to the sndif protocol and then will send the updated version of the driver for the further review. This message has below line in its header. > In-Reply-To: <e56a09e9-da66-b748-4e82-4b96a18ce...@gmail.com> This field is defined in RFC822[1], and recent mail clients use this header field to associate the message to a message which the field indicates. This results in a series of messages, so-called 'message thread'. Iwai-san would like you to start a new message thread for your topic. Would you please post this message again without the header field? Generally, receiving no reactions means that readers/reviewers don't get enough information for your idea yet. (Of course, there's a probability that your work attracts no one...) In this case, submitting more resources is better, rather than requesting comments to them. For instance, you can point links to backend/frontend implementation as para-virtualization drivers which use the new feature of interface, if you did work for it. Indicating procedure to use a series of your work is better for test, if possible. [1] https://www.ietf.org/rfc/rfc0822.txt Regards Takashi Sakamoto
Re: [RFC] ALSA: vsnd: Add Xen para-virtualized frontend driver
On Oct 30 2017 15:33, Oleksandr Andrushchenko wrote: This is an attempt to summarize previous discussions on Xen para-virtual sound driver. A first attempt has been made to upstream the driver [1] which brought number of fundamental questions, one of the biggest ones was that the frontend driver has no means to synchronize its period elapsed event with the host driver, but uses software emulation on the guest side [2] with a timer. In order to address this a change to the existing Xen para-virtual sound protocol [3] was proposed to fill this gap [4] and remove emulation: 1. Introduced a new event channel from back to front 2. New event with number of bytes played/captured (XENSND_EVT_CUR_POS, to be used for sending snd_pcm_period_elapsed at frontend (in Linux implementation, sent in bytes, not frames to make the protocol generic and consistent) 3. New request for playback/capture control (XENSND_OP_TRIGGER) with start/pause/stop/resume sub-ops. Along with these changes other comments on the driver were addressed, e.g. split into smaller chunks, moved the driver from misc to xen etc. [5]. Hope, this helps to get the full picture of what was discussed and makes it possible to move forward: if the approach seems ok, then I'll start upstreaming the changes to the sndif protocol and then will send the updated version of the driver for the further review. This message has below line in its header. > In-Reply-To: This field is defined in RFC822[1], and recent mail clients use this header field to associate the message to a message which the field indicates. This results in a series of messages, so-called 'message thread'. Iwai-san would like you to start a new message thread for your topic. Would you please post this message again without the header field? Generally, receiving no reactions means that readers/reviewers don't get enough information for your idea yet. (Of course, there's a probability that your work attracts no one...) In this case, submitting more resources is better, rather than requesting comments to them. For instance, you can point links to backend/frontend implementation as para-virtualization drivers which use the new feature of interface, if you did work for it. Indicating procedure to use a series of your work is better for test, if possible. [1] https://www.ietf.org/rfc/rfc0822.txt Regards Takashi Sakamoto
Re: 4.13 regression: get_kctl_0dB_offset doesn't handle all possible callbacks
Hi,
On Oct 14 2017 07:46, PaX Team wrote:
> what KERNEXEC on i386 does is that it executes kernel code in its own 0-based
> code segment hence the 'low' code addresses. due to the current logic that
> checks for SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK in get_kctl_0dB_offset, this
> callback address is instead treated as a data pointer (as apparently
> SNDRV_CTL_ELEM_ACCESS_TLV_READ is also set) and since it's not a valid kernel
> address, it causes a GPF under the UDEREF PaX feature (without UDEREF it'd
> have been an oops since such low addresses are not mapped for kernel threads).
There're two ways to use 'struct snd_kcontrol.tlv'; constant array (=.p) an
a handler (= .c). An 'access' flag in each member of
'struct snd_kcontrol.vd' represent which way is used for the member.
Therefore, as long as checking the flag, the 'get_kctl_0dB_offset()' doesn't
handle a pointer to the handler as a pointer to array of TLV container.
> on vanilla kernels all this is a silent read of kernel code bytes that are
> then interpreted as the tlv[] array content, which is probably not what you
> want either.
Yes. Current code include a bug of inappropriate condition statement for the
above. As a result, it allows to handle a pointer to the handler as a
pointer to TLV data.
> as for fixing this, first the above mentioned assumption should be
> re-evaluated.
> if it's considered correct then there is some logic bug in my case (i can help
> debug it if you tell me what to do) otherwise the current pattern of
>
> if (SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK && callback ==
> snd_hda_mixer_amp_tlv) {
> call wrapped function underneath snd_hda_mixer_amp_tlv
> } else if (SNDRV_CTL_ELEM_ACCESS_TLV_READ) {
> treat unrecognized callback address as data ptr
> }
>
> should be changed to
>
> if (SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK( {
> if (callback == snd_hda_mixer_amp_tlv) {
> call wrapped function underneath snd_hda_mixer_amp_tlv
> } else if (callback == others) {
> handle others, WARN/BUG/etc
> }
> } else if (SNDRV_CTL_ELEM_ACCESS_TLV_READ) {
> no longer treat unrecognized callback address as data ptr
> }
Good enough as a solution. Please test a patch in the end of this message.
> and all other callbacks with userland access should be refactored the same
> way as snd_hda_mixer_amp_tlv was. i'd also suggest that you at least do the
> above suggested if/else pattern change in order to prevent the misuse of
> unexpected callbacks in the future.
This suggestion is better for safety. Do you have some ways to detect the
pattern on current vanilla kernel? Or we should find it by eye-grep?
Thanks
Takashi Sakamoto
8<
>From 85896b50aa22bf2f2b5e45456daa16d386602edc Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-taka...@sakamocchi.jp>
Date: Sat, 14 Oct 2017 14:08:51 +0900
Subject: [PATCH] ALSA: hda-intel: Fix to handle a pointer to TLV handler
as a pointer to TLV data
In a design of ALSA control core, an 'access' flag on each entry of
'struct snd_kcontrol.vd' represents the type of 'struct snd_kcontrol.tlv'
for the corresponding element; a pointer to TLV data
(!=SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) or a pointer to handler
(==SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK).
In current implementation of 'get_kctl_0dB_offset()', condition statement
is not proper to distinguish these two cases. As a result, it handles
a pointer to the handler as a pointer to TLV data for some control element
sets. This bug brings invalid references to kernel space. A reporter shows
a sample of backtrace.
PAX: suspicious general protection fault: [#1] PREEMPT SMP
Modules linked in:
CPU: 4 PID: 31 Comm: kworker/4:0 Not tainted 4.13.5-i386-pax #17
Workqueue: events azx_probe_work
task: eb61c880 task.stack: eb62a000
EIP: 0060:[<009bbd2d>] init_slave_0dB+0x2d/0xa0
EFLAGS: 00210202 CPU: 4
EAX: 00991fd0 EBX: e9883a80 ECX: e9883a80 EDX: eb62bd74
ESI: eb62bd74 EDI: e9098800 EBP: eb62bd08 ESP: eb62bcec
DS: 0068 ES: 0068 FS: 00d8 GS: 0068 SS: 0068
CR0: 80050033 CR2: CR3: 03b2d100 CR4: 000406f0 shadow CR4: 000406f0
Call Trace:
[<009bb469>] map_slaves+0xb9/0xe0
[<009bce0e>] __snd_hda_add_vmaster+0xde/0x110
[<009c82f0>] snd_hda_gen_build_controls+0x1a0/0x1b0
[<009d0a4d>] cx_auto_build_controls+0xd/0x70
[<009bdf76>] snd_hda_codec_build_controls+0x186/0x1d0
[<009b92ad>] hda_codec_driver_probe+0x6d/0xf0
[<006a3be9>] driver_probe_device+0x289/0x420
[<006a3ed6>] __device_attach_driver+0x76/0x100
[<006a1edf>] bus_for_each_drv+0x3f/0x70
[<006a3813>] __device_attach+0xa3/0x110
[<006a3f9d>] device_initial_probe+0xd/0x10
[<006a2d6f>] bus_probe_device+0x6f/0x80
[<006a100f>] device_add+0x2cf/0x590
[<009d8d8c>] snd_hdac_device_register+0xc/0x40
[<009
Re: 4.13 regression: get_kctl_0dB_offset doesn't handle all possible callbacks
Hi,
On Oct 14 2017 07:46, PaX Team wrote:
> what KERNEXEC on i386 does is that it executes kernel code in its own 0-based
> code segment hence the 'low' code addresses. due to the current logic that
> checks for SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK in get_kctl_0dB_offset, this
> callback address is instead treated as a data pointer (as apparently
> SNDRV_CTL_ELEM_ACCESS_TLV_READ is also set) and since it's not a valid kernel
> address, it causes a GPF under the UDEREF PaX feature (without UDEREF it'd
> have been an oops since such low addresses are not mapped for kernel threads).
There're two ways to use 'struct snd_kcontrol.tlv'; constant array (=.p) an
a handler (= .c). An 'access' flag in each member of
'struct snd_kcontrol.vd' represent which way is used for the member.
Therefore, as long as checking the flag, the 'get_kctl_0dB_offset()' doesn't
handle a pointer to the handler as a pointer to array of TLV container.
> on vanilla kernels all this is a silent read of kernel code bytes that are
> then interpreted as the tlv[] array content, which is probably not what you
> want either.
Yes. Current code include a bug of inappropriate condition statement for the
above. As a result, it allows to handle a pointer to the handler as a
pointer to TLV data.
> as for fixing this, first the above mentioned assumption should be
> re-evaluated.
> if it's considered correct then there is some logic bug in my case (i can help
> debug it if you tell me what to do) otherwise the current pattern of
>
> if (SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK && callback ==
> snd_hda_mixer_amp_tlv) {
> call wrapped function underneath snd_hda_mixer_amp_tlv
> } else if (SNDRV_CTL_ELEM_ACCESS_TLV_READ) {
> treat unrecognized callback address as data ptr
> }
>
> should be changed to
>
> if (SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK( {
> if (callback == snd_hda_mixer_amp_tlv) {
> call wrapped function underneath snd_hda_mixer_amp_tlv
> } else if (callback == others) {
> handle others, WARN/BUG/etc
> }
> } else if (SNDRV_CTL_ELEM_ACCESS_TLV_READ) {
> no longer treat unrecognized callback address as data ptr
> }
Good enough as a solution. Please test a patch in the end of this message.
> and all other callbacks with userland access should be refactored the same
> way as snd_hda_mixer_amp_tlv was. i'd also suggest that you at least do the
> above suggested if/else pattern change in order to prevent the misuse of
> unexpected callbacks in the future.
This suggestion is better for safety. Do you have some ways to detect the
pattern on current vanilla kernel? Or we should find it by eye-grep?
Thanks
Takashi Sakamoto
8<
>From 85896b50aa22bf2f2b5e45456daa16d386602edc Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto
Date: Sat, 14 Oct 2017 14:08:51 +0900
Subject: [PATCH] ALSA: hda-intel: Fix to handle a pointer to TLV handler
as a pointer to TLV data
In a design of ALSA control core, an 'access' flag on each entry of
'struct snd_kcontrol.vd' represents the type of 'struct snd_kcontrol.tlv'
for the corresponding element; a pointer to TLV data
(!=SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) or a pointer to handler
(==SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK).
In current implementation of 'get_kctl_0dB_offset()', condition statement
is not proper to distinguish these two cases. As a result, it handles
a pointer to the handler as a pointer to TLV data for some control element
sets. This bug brings invalid references to kernel space. A reporter shows
a sample of backtrace.
PAX: suspicious general protection fault: [#1] PREEMPT SMP
Modules linked in:
CPU: 4 PID: 31 Comm: kworker/4:0 Not tainted 4.13.5-i386-pax #17
Workqueue: events azx_probe_work
task: eb61c880 task.stack: eb62a000
EIP: 0060:[<009bbd2d>] init_slave_0dB+0x2d/0xa0
EFLAGS: 00210202 CPU: 4
EAX: 00991fd0 EBX: e9883a80 ECX: e9883a80 EDX: eb62bd74
ESI: eb62bd74 EDI: e9098800 EBP: eb62bd08 ESP: eb62bcec
DS: 0068 ES: 0068 FS: 00d8 GS: 0068 SS: 0068
CR0: 80050033 CR2: CR3: 03b2d100 CR4: 000406f0 shadow CR4: 000406f0
Call Trace:
[<009bb469>] map_slaves+0xb9/0xe0
[<009bce0e>] __snd_hda_add_vmaster+0xde/0x110
[<009c82f0>] snd_hda_gen_build_controls+0x1a0/0x1b0
[<009d0a4d>] cx_auto_build_controls+0xd/0x70
[<009bdf76>] snd_hda_codec_build_controls+0x186/0x1d0
[<009b92ad>] hda_codec_driver_probe+0x6d/0xf0
[<006a3be9>] driver_probe_device+0x289/0x420
[<006a3ed6>] __device_attach_driver+0x76/0x100
[<006a1edf>] bus_for_each_drv+0x3f/0x70
[<006a3813>] __device_attach+0xa3/0x110
[<006a3f9d>] device_initial_probe+0xd/0x10
[<006a2d6f>] bus_probe_device+0x6f/0x80
[<006a100f>] device_add+0x2cf/0x590
[<009d8d8c>] snd_hdac_device_register+0xc/0x40
[<009b90b4>] snd_hda_codec_co
Re: ALSA: bebob: Use common error handling code in snd_bebob_stream_start_duplex()
On Sep 27 2017 15:38, SF Markus Elfring wrote: As long as I know, the last product with this solution was shipped at 2010. Thus the driver is under maintenance. Thanks for your information. I have some tasks for this driver as well as drivers in ALSA firewire stack, however basically this driver is under maintenance and might not get further integration. Good to know in principle … I think that code cleanup for the driver don't help the other developers. I thought in an other direction if other contributors would like to care for longer term support. It's better to apply such cleanup to more long-live codes such as core functionality of ALSA. I am trying another general transformation pattern out. In this point of view, whether your patchset is worth to be applied or not, Please keep enough time to think about. Automatic source code analysis can find various update candidates. I am just unsure if a bit more software development attention is still appropriate at some places. Do you find any of the affected software modules so outdated so that they should not be touched any more? Please don't ignore the practicality I've explained, and don't accumulate your questions without it. Regards Takashi Sakamoto
Re: ALSA: bebob: Use common error handling code in snd_bebob_stream_start_duplex()
On Sep 27 2017 15:38, SF Markus Elfring wrote: As long as I know, the last product with this solution was shipped at 2010. Thus the driver is under maintenance. Thanks for your information. I have some tasks for this driver as well as drivers in ALSA firewire stack, however basically this driver is under maintenance and might not get further integration. Good to know in principle … I think that code cleanup for the driver don't help the other developers. I thought in an other direction if other contributors would like to care for longer term support. It's better to apply such cleanup to more long-live codes such as core functionality of ALSA. I am trying another general transformation pattern out. In this point of view, whether your patchset is worth to be applied or not, Please keep enough time to think about. Automatic source code analysis can find various update candidates. I am just unsure if a bit more software development attention is still appropriate at some places. Do you find any of the affected software modules so outdated so that they should not be touched any more? Please don't ignore the practicality I've explained, and don't accumulate your questions without it. Regards Takashi Sakamoto
Re: [PATCH] ALSA: line6: make snd_pcm_ops const
On Sep 27 2017 15:19, Bhumika Goyal wrote:
Make these const as they are only passed to a const argument of the
function snd_pcm_set_ops in the file referencing them. Also, add const
to the declaration in the headers.
Structures found using Coccinelle and changes done by hand.
Signed-off-by: Bhumika Goyal <bhumi...@gmail.com>
---
sound/usb/line6/capture.c | 2 +-
sound/usb/line6/capture.h | 2 +-
sound/usb/line6/playback.c | 2 +-
sound/usb/line6/playback.h | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
This looks good to me.
Reviewed-by: Takashi Sakamoto <o-taka...@sakamocchi.jp>
diff --git a/sound/usb/line6/capture.c b/sound/usb/line6/capture.c
index 7c81256..947d616 100644
--- a/sound/usb/line6/capture.c
+++ b/sound/usb/line6/capture.c
@@ -248,7 +248,7 @@ static int snd_line6_capture_close(struct snd_pcm_substream
*substream)
}
/* capture operators */
-struct snd_pcm_ops snd_line6_capture_ops = {
+const struct snd_pcm_ops snd_line6_capture_ops = {
.open = snd_line6_capture_open,
.close = snd_line6_capture_close,
.ioctl = snd_pcm_lib_ioctl,
diff --git a/sound/usb/line6/capture.h b/sound/usb/line6/capture.h
index 890b21b..b67ccc3 100644
--- a/sound/usb/line6/capture.h
+++ b/sound/usb/line6/capture.h
@@ -17,7 +17,7 @@
#include "driver.h"
#include "pcm.h"
-extern struct snd_pcm_ops snd_line6_capture_ops;
+extern const struct snd_pcm_ops snd_line6_capture_ops;
extern void line6_capture_copy(struct snd_line6_pcm *line6pcm, char *fbuf,
int fsize);
diff --git a/sound/usb/line6/playback.c b/sound/usb/line6/playback.c
index 812d181..819e9b2 100644
--- a/sound/usb/line6/playback.c
+++ b/sound/usb/line6/playback.c
@@ -393,7 +393,7 @@ static int snd_line6_playback_close(struct
snd_pcm_substream *substream)
}
/* playback operators */
-struct snd_pcm_ops snd_line6_playback_ops = {
+const struct snd_pcm_ops snd_line6_playback_ops = {
.open = snd_line6_playback_open,
.close = snd_line6_playback_close,
.ioctl = snd_pcm_lib_ioctl,
diff --git a/sound/usb/line6/playback.h b/sound/usb/line6/playback.h
index 51fce29..d8d3b8a 100644
--- a/sound/usb/line6/playback.h
+++ b/sound/usb/line6/playback.h
@@ -27,7 +27,7 @@
*/
#define USE_CLEAR_BUFFER_WORKAROUND 1
-extern struct snd_pcm_ops snd_line6_playback_ops;
+extern const struct snd_pcm_ops snd_line6_playback_ops;
extern int line6_create_audio_out_urbs(struct snd_line6_pcm *line6pcm);
extern int line6_submit_audio_out_all_urbs(struct snd_line6_pcm *line6pcm);
Thanks
Takashi Sakamoto
Re: [PATCH] ALSA: line6: make snd_pcm_ops const
On Sep 27 2017 15:19, Bhumika Goyal wrote:
Make these const as they are only passed to a const argument of the
function snd_pcm_set_ops in the file referencing them. Also, add const
to the declaration in the headers.
Structures found using Coccinelle and changes done by hand.
Signed-off-by: Bhumika Goyal
---
sound/usb/line6/capture.c | 2 +-
sound/usb/line6/capture.h | 2 +-
sound/usb/line6/playback.c | 2 +-
sound/usb/line6/playback.h | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
This looks good to me.
Reviewed-by: Takashi Sakamoto
diff --git a/sound/usb/line6/capture.c b/sound/usb/line6/capture.c
index 7c81256..947d616 100644
--- a/sound/usb/line6/capture.c
+++ b/sound/usb/line6/capture.c
@@ -248,7 +248,7 @@ static int snd_line6_capture_close(struct snd_pcm_substream
*substream)
}
/* capture operators */
-struct snd_pcm_ops snd_line6_capture_ops = {
+const struct snd_pcm_ops snd_line6_capture_ops = {
.open = snd_line6_capture_open,
.close = snd_line6_capture_close,
.ioctl = snd_pcm_lib_ioctl,
diff --git a/sound/usb/line6/capture.h b/sound/usb/line6/capture.h
index 890b21b..b67ccc3 100644
--- a/sound/usb/line6/capture.h
+++ b/sound/usb/line6/capture.h
@@ -17,7 +17,7 @@
#include "driver.h"
#include "pcm.h"
-extern struct snd_pcm_ops snd_line6_capture_ops;
+extern const struct snd_pcm_ops snd_line6_capture_ops;
extern void line6_capture_copy(struct snd_line6_pcm *line6pcm, char *fbuf,
int fsize);
diff --git a/sound/usb/line6/playback.c b/sound/usb/line6/playback.c
index 812d181..819e9b2 100644
--- a/sound/usb/line6/playback.c
+++ b/sound/usb/line6/playback.c
@@ -393,7 +393,7 @@ static int snd_line6_playback_close(struct
snd_pcm_substream *substream)
}
/* playback operators */
-struct snd_pcm_ops snd_line6_playback_ops = {
+const struct snd_pcm_ops snd_line6_playback_ops = {
.open = snd_line6_playback_open,
.close = snd_line6_playback_close,
.ioctl = snd_pcm_lib_ioctl,
diff --git a/sound/usb/line6/playback.h b/sound/usb/line6/playback.h
index 51fce29..d8d3b8a 100644
--- a/sound/usb/line6/playback.h
+++ b/sound/usb/line6/playback.h
@@ -27,7 +27,7 @@
*/
#define USE_CLEAR_BUFFER_WORKAROUND 1
-extern struct snd_pcm_ops snd_line6_playback_ops;
+extern const struct snd_pcm_ops snd_line6_playback_ops;
extern int line6_create_audio_out_urbs(struct snd_line6_pcm *line6pcm);
extern int line6_submit_audio_out_all_urbs(struct snd_line6_pcm *line6pcm);
Thanks
Takashi Sakamoto
Re: ALSA: bebob: Use common error handling code in snd_bebob_stream_start_duplex()
Hi,
On Sep 24 2017 16:06, SF Markus Elfring wrote:
668 if (!amdtp_stream_wait_callback(>tx_stream,
669 CALLBACK_TIMEOUT)) {
670 amdtp_stream_stop(>tx_stream);
671 amdtp_stream_stop(>rx_stream);
672 break_both_connections(bebob);
673 err = -ETIMEDOUT;
674 }
675 }
I think it better to apply your solution too in the above to keep code
consistency.
How do you think about to adjust this function implementation after the other
two
update steps from the patch series would be integrated?
For the other patches, I can find no merit to apply except for reduction
of the number of characters included in the file.
Would you like to refer to any specific update suggestions for further
clarification?
I had already read your patch comments and understand your intention
somehow, because it's a part of task for daily reviewing. Then, I did
comment.
At development for Linux kernel, there're some important points for
developers to notice. In our case, we should care of _practicality_.
Roughly speaking, our work for kernel should add advantages for
kernel/application runtime or assists the other developers' work. In
this point, some patches are welcome and the others are not. I'll show
you two examples.
In this subsystem, I have reviewed patches from Julia Lawall. The most
of her or his patches attempt to add 'const' qualifier for read-only
symbols. As a result, the symbols place to '.rodata' section of ELF
binary. This section has a hint for loaders to put these symbols to
segments with read-only attributes. This has an advantage for
compilation time and runtime. Recent compilers can detect codes to
change content of the symbols with 'const' qualifier. Even if the codes
were exposed from developers understanding or compilers' check,
segmentation would occur in runtime at early stage of development or
early days after releasing kernel. This is very helpful for developers
to find unexpected changes for contents of the symbol.
I also subscribe a mailing list of Linux Driver Project[1], which is for
various drivers. Sometimes posted patches are rejected by maintainers.
Such patches typically include minor code change without actual
advantages for runtime or developers. For instance, patches for '80
characters per line' are often posted and rejected. If this kind of
patchset were added to change history of kernel, tons of unpractical
logs are accumulated for development. This make it worse for developers
to maintain the kernel.
By the way, ALSA BeBoB driver is just for ASIC and firmwares which
ArchWave AG (formerly BridgeCo. AG.) had produced for devices with
IEC 61883-1/6 packet streaming on IEEE 1394 bus and Time Sensitive
Network (TSN). As long as I know, the last product with this solution
was shipped at 2010. Thus the driver is under maintenance. I have some
tasks for this driver as well as drivers in ALSA firewire stack,
however basically this driver is under maintenance and might not get
further integration. I think that code cleanup for the driver don't help
the other developers. It's better to apply such cleanup to more
long-live codes such as core functionality of ALSA. (But pay enough
attention to practicality of the changes when you start this kind of
work.)
In this point of view, whether your patchset is worth to be applied
or not, Please keep enough time to think about.
[1] http://driverdev.linuxdriverproject.org/mailman/listinfo
Thanks
Takashi Sakamoto
Re: ALSA: bebob: Use common error handling code in snd_bebob_stream_start_duplex()
Hi,
On Sep 24 2017 16:06, SF Markus Elfring wrote:
668 if (!amdtp_stream_wait_callback(>tx_stream,
669 CALLBACK_TIMEOUT)) {
670 amdtp_stream_stop(>tx_stream);
671 amdtp_stream_stop(>rx_stream);
672 break_both_connections(bebob);
673 err = -ETIMEDOUT;
674 }
675 }
I think it better to apply your solution too in the above to keep code
consistency.
How do you think about to adjust this function implementation after the other
two
update steps from the patch series would be integrated?
For the other patches, I can find no merit to apply except for reduction
of the number of characters included in the file.
Would you like to refer to any specific update suggestions for further
clarification?
I had already read your patch comments and understand your intention
somehow, because it's a part of task for daily reviewing. Then, I did
comment.
At development for Linux kernel, there're some important points for
developers to notice. In our case, we should care of _practicality_.
Roughly speaking, our work for kernel should add advantages for
kernel/application runtime or assists the other developers' work. In
this point, some patches are welcome and the others are not. I'll show
you two examples.
In this subsystem, I have reviewed patches from Julia Lawall. The most
of her or his patches attempt to add 'const' qualifier for read-only
symbols. As a result, the symbols place to '.rodata' section of ELF
binary. This section has a hint for loaders to put these symbols to
segments with read-only attributes. This has an advantage for
compilation time and runtime. Recent compilers can detect codes to
change content of the symbols with 'const' qualifier. Even if the codes
were exposed from developers understanding or compilers' check,
segmentation would occur in runtime at early stage of development or
early days after releasing kernel. This is very helpful for developers
to find unexpected changes for contents of the symbol.
I also subscribe a mailing list of Linux Driver Project[1], which is for
various drivers. Sometimes posted patches are rejected by maintainers.
Such patches typically include minor code change without actual
advantages for runtime or developers. For instance, patches for '80
characters per line' are often posted and rejected. If this kind of
patchset were added to change history of kernel, tons of unpractical
logs are accumulated for development. This make it worse for developers
to maintain the kernel.
By the way, ALSA BeBoB driver is just for ASIC and firmwares which
ArchWave AG (formerly BridgeCo. AG.) had produced for devices with
IEC 61883-1/6 packet streaming on IEEE 1394 bus and Time Sensitive
Network (TSN). As long as I know, the last product with this solution
was shipped at 2010. Thus the driver is under maintenance. I have some
tasks for this driver as well as drivers in ALSA firewire stack,
however basically this driver is under maintenance and might not get
further integration. I think that code cleanup for the driver don't help
the other developers. It's better to apply such cleanup to more
long-live codes such as core functionality of ALSA. (But pay enough
attention to practicality of the changes when you start this kind of
work.)
In this point of view, whether your patchset is worth to be applied
or not, Please keep enough time to think about.
[1] http://driverdev.linuxdriverproject.org/mailman/listinfo
Thanks
Takashi Sakamoto
Re: [PATCH 1/3] ALSA: bebob: Use common error handling code in snd_bebob_stream_start_duplex()
Hi,
On Sep 6 2017 19:22, SF Markus Elfring wrote:
From: Markus Elfring <elfr...@users.sourceforge.net>
Date: Wed, 6 Sep 2017 11:40:53 +0200
Add jump targets so that a bit of exception handling can be better reused
at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
---
sound/firewire/bebob/bebob_stream.c | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/sound/firewire/bebob/bebob_stream.c
b/sound/firewire/bebob/bebob_stream.c
index 4d3034a68bdf..bc9e42b6368e 100644
--- a/sound/firewire/bebob/bebob_stream.c
+++ b/sound/firewire/bebob/bebob_stream.c
...
@@ -666,9 +661,7 @@ int snd_bebob_stream_start_duplex(struct snd_bebob *bebob,
unsigned int rate)
if (err < 0) {
dev_err(>unit->device,
"fail to run AMDTP slave stream:%d\n", err);
- amdtp_stream_stop(>rx_stream);
- break_both_connections(bebob);
- goto end;
+ goto stop_rx_stream;
}
/* wait first callback */
After the above code block, we have below code block.
658 /* start slave if needed */
659 if (!amdtp_stream_running(>tx_stream)) {
660 err = start_stream(bebob, >tx_stream, rate);
661 if (err < 0) {
662 dev_err(>unit->device,
663 "fail to run AMDTP slave stream:%d\n", err);
664 goto stop_rx_stream;
665 }
666
667 /* wait first callback */
668 if (!amdtp_stream_wait_callback(>tx_stream,
669 CALLBACK_TIMEOUT)) {
670 amdtp_stream_stop(>tx_stream);
671 amdtp_stream_stop(>rx_stream);
672 break_both_connections(bebob);
673 err = -ETIMEDOUT;
674 }
675 }
I think it better to apply your solution too in the above to keep code
consistency.
@@ -682,6 +675,12 @@ int snd_bebob_stream_start_duplex(struct snd_bebob *bebob,
unsigned int rate)
}
end:
return err;
+
+stop_rx_stream:
+ amdtp_stream_stop(>rx_stream);
+break_connections:
+ break_both_connections(bebob);
+ return err;
}
void snd_bebob_stream_stop_duplex(struct snd_bebob *bebob)
For the other patches, I can find no merit to apply except for reduction
of the number of characters included in the file.
Thanks
Takashi Sakamoto
Re: [PATCH 1/3] ALSA: bebob: Use common error handling code in snd_bebob_stream_start_duplex()
Hi,
On Sep 6 2017 19:22, SF Markus Elfring wrote:
From: Markus Elfring
Date: Wed, 6 Sep 2017 11:40:53 +0200
Add jump targets so that a bit of exception handling can be better reused
at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
sound/firewire/bebob/bebob_stream.c | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/sound/firewire/bebob/bebob_stream.c
b/sound/firewire/bebob/bebob_stream.c
index 4d3034a68bdf..bc9e42b6368e 100644
--- a/sound/firewire/bebob/bebob_stream.c
+++ b/sound/firewire/bebob/bebob_stream.c
...
@@ -666,9 +661,7 @@ int snd_bebob_stream_start_duplex(struct snd_bebob *bebob,
unsigned int rate)
if (err < 0) {
dev_err(>unit->device,
"fail to run AMDTP slave stream:%d\n", err);
- amdtp_stream_stop(>rx_stream);
- break_both_connections(bebob);
- goto end;
+ goto stop_rx_stream;
}
/* wait first callback */
After the above code block, we have below code block.
658 /* start slave if needed */
659 if (!amdtp_stream_running(>tx_stream)) {
660 err = start_stream(bebob, >tx_stream, rate);
661 if (err < 0) {
662 dev_err(>unit->device,
663 "fail to run AMDTP slave stream:%d\n", err);
664 goto stop_rx_stream;
665 }
666
667 /* wait first callback */
668 if (!amdtp_stream_wait_callback(>tx_stream,
669 CALLBACK_TIMEOUT)) {
670 amdtp_stream_stop(>tx_stream);
671 amdtp_stream_stop(>rx_stream);
672 break_both_connections(bebob);
673 err = -ETIMEDOUT;
674 }
675 }
I think it better to apply your solution too in the above to keep code
consistency.
@@ -682,6 +675,12 @@ int snd_bebob_stream_start_duplex(struct snd_bebob *bebob,
unsigned int rate)
}
end:
return err;
+
+stop_rx_stream:
+ amdtp_stream_stop(>rx_stream);
+break_connections:
+ break_both_connections(bebob);
+ return err;
}
void snd_bebob_stream_stop_duplex(struct snd_bebob *bebob)
For the other patches, I can find no merit to apply except for reduction
of the number of characters included in the file.
Thanks
Takashi Sakamoto
Re: [RFC PATCH 0/7] Fix year 2038 issue for sound subsystem
Hi,
On Sep 21 2017 15:18, Baolin Wang wrote:
Since many structures will use timespec type variables to record time stamp
in uapi/asound.h, which are not year 2038 safe on 32bit system. This patchset
tries to introduce new structures removing timespec type to compatible native
mode and compat mode.
Moreover this patchset also converts the internal structrures to use timespec64
type and related APIs.
Baolin Wang (7):
sound: Replace timespec with timespec64
sound: core: Avoid using timespec for struct snd_pcm_status
sound: core: Avoid using timespec for struct snd_pcm_sync_ptr
sound: core: Avoid using timespec for struct snd_rawmidi_status
sound: core: Avoid using timespec for struct snd_timer_status
uapi: sound: Avoid using timespec for struct snd_ctl_elem_value
sound: core: Avoid using timespec for struct snd_timer_tread
include/sound/pcm.h | 113 -
include/sound/timer.h |4 +-
include/uapi/sound/asound.h | 15 +-
sound/core/pcm.c | 14 +-
sound/core/pcm_compat.c | 466 +
sound/core/pcm_lib.c | 33 +--
sound/core/pcm_native.c | 227 ++
sound/core/rawmidi.c | 74 +-
sound/core/rawmidi_compat.c | 90 +--
sound/core/timer.c| 247
sound/core/timer_compat.c | 25 +-
sound/pci/hda/hda_controller.c| 10 +-
sound/soc/intel/skylake/skl-pcm.c |4 +-
13 files changed, 1046 insertions(+), 276 deletions(-)
I'm a minor Takashi in this subsystem and not those who you'd like to
talk about this issue. But I have interests in it and would like to
assist you, as long as I can do for it.
As a nitpicking, your patchset brings compilation error at
configurations for x86, and x86-64 with x32 ABI support.
## x86-64 architecture and amd64 ABI support
CONFIG_64BIT=y
CONFIG_X86_64=y
Success.
## x86-64 architecture and amd64/x32 ABI support
CONFIG_64BIT=y
CONFIG_X86_64=y
CONFIG_X86_X32=y
```
sound/core/timer_compat.c:124:54: error: array type has incomplete
element type 'struct snd_timer_status32'
SNDRV_TIMER_IOCTL_STATUS32 = _IOW('T', 0x14, struct snd_timer_status32),
```
This error comes from a commit 1229cccbefe7 ('sound: core: Avoid using
timespec for struct snd_timer_status').
```
sound/core/pcm_compat.c: In function 'snd_pcm_ioctl_sync_ptr_compat':
sound/core/pcm_compat.c:623:9: error: assignment from incompatible
pointer type [-Werror=incompatible-pointer-types]
status = runtime->status;
^
sound/core/pcm_compat.c: In function 'snd_pcm_ioctl_sync_ptr_x32':
sound/core/pcm_compat.c:711:9: error: assignment from incompatible
pointer type [-Werror=incompatible-pointer-types]
status = runtime->status;
```
This error comes from a commit 947c463adc00('sound: core: Avoid using
timespec
for struct snd_pcm_status').
## x86 architecture and i386 ABI support
CONFIG_X86_32=y
```
sound/core/pcm_native.c: In function 'snd_pcm_common_ioctl':
sound/core/pcm_native.c:3065:2: error: duplicate case value
case SNDRV_PCM_IOCTL_SYNC_PTR64:
^~~~
sound/core/pcm_native.c:3062:2: error: previously used here
case SNDRV_PCM_IOCTL_SYNC_PTR32:
```
This error comes from a commit c0513348a7b39 ('sound: core: Avoid using
timespec for struct snd_pcm_sync_ptr').
Your patchset brought conflicts to 'for-next' branch in a repository
which Iwai-san maintains[1]. I rebased your patchset on a commit
729fbfc92a45 ('ALSA: line6: add support for POD HD DESKTOP') which is a
HEAD of 'for-next' branch and pushed into my repository on github[2].
I respect your work for this issue, however it's better to check whether
your patchset is buildable or not on major configurations before
posting.
I note that at a development period for v4.5 kernel, ALSA developers
(mainly Iwai-san) fixed x32 ABI compatibility bugs. Then I prepared for
a rough set of test for ioctl command[3] to check his work. The set will
partly help your work, I think (but it's really rough).
I need more time for reviewing. At least, this week is for recovery from
my tough work to rewrite aplay[4].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
[2] https://github.com/takaswie/sound/tree/topic/year2038-rfc1
[3] https://github.com/takaswie/alsa-ioctl-test/
[4] [alsa-devel] [RFCv2][PATCH 00/38] alsa-utils: axfer: rewrite aplay
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-September/125574.html
Thanks
Takashi Sakamoto
Re: [RFC PATCH 0/7] Fix year 2038 issue for sound subsystem
Hi,
On Sep 21 2017 15:18, Baolin Wang wrote:
Since many structures will use timespec type variables to record time stamp
in uapi/asound.h, which are not year 2038 safe on 32bit system. This patchset
tries to introduce new structures removing timespec type to compatible native
mode and compat mode.
Moreover this patchset also converts the internal structrures to use timespec64
type and related APIs.
Baolin Wang (7):
sound: Replace timespec with timespec64
sound: core: Avoid using timespec for struct snd_pcm_status
sound: core: Avoid using timespec for struct snd_pcm_sync_ptr
sound: core: Avoid using timespec for struct snd_rawmidi_status
sound: core: Avoid using timespec for struct snd_timer_status
uapi: sound: Avoid using timespec for struct snd_ctl_elem_value
sound: core: Avoid using timespec for struct snd_timer_tread
include/sound/pcm.h | 113 -
include/sound/timer.h |4 +-
include/uapi/sound/asound.h | 15 +-
sound/core/pcm.c | 14 +-
sound/core/pcm_compat.c | 466 +
sound/core/pcm_lib.c | 33 +--
sound/core/pcm_native.c | 227 ++
sound/core/rawmidi.c | 74 +-
sound/core/rawmidi_compat.c | 90 +--
sound/core/timer.c| 247
sound/core/timer_compat.c | 25 +-
sound/pci/hda/hda_controller.c| 10 +-
sound/soc/intel/skylake/skl-pcm.c |4 +-
13 files changed, 1046 insertions(+), 276 deletions(-)
I'm a minor Takashi in this subsystem and not those who you'd like to
talk about this issue. But I have interests in it and would like to
assist you, as long as I can do for it.
As a nitpicking, your patchset brings compilation error at
configurations for x86, and x86-64 with x32 ABI support.
## x86-64 architecture and amd64 ABI support
CONFIG_64BIT=y
CONFIG_X86_64=y
Success.
## x86-64 architecture and amd64/x32 ABI support
CONFIG_64BIT=y
CONFIG_X86_64=y
CONFIG_X86_X32=y
```
sound/core/timer_compat.c:124:54: error: array type has incomplete
element type 'struct snd_timer_status32'
SNDRV_TIMER_IOCTL_STATUS32 = _IOW('T', 0x14, struct snd_timer_status32),
```
This error comes from a commit 1229cccbefe7 ('sound: core: Avoid using
timespec for struct snd_timer_status').
```
sound/core/pcm_compat.c: In function 'snd_pcm_ioctl_sync_ptr_compat':
sound/core/pcm_compat.c:623:9: error: assignment from incompatible
pointer type [-Werror=incompatible-pointer-types]
status = runtime->status;
^
sound/core/pcm_compat.c: In function 'snd_pcm_ioctl_sync_ptr_x32':
sound/core/pcm_compat.c:711:9: error: assignment from incompatible
pointer type [-Werror=incompatible-pointer-types]
status = runtime->status;
```
This error comes from a commit 947c463adc00('sound: core: Avoid using
timespec
for struct snd_pcm_status').
## x86 architecture and i386 ABI support
CONFIG_X86_32=y
```
sound/core/pcm_native.c: In function 'snd_pcm_common_ioctl':
sound/core/pcm_native.c:3065:2: error: duplicate case value
case SNDRV_PCM_IOCTL_SYNC_PTR64:
^~~~
sound/core/pcm_native.c:3062:2: error: previously used here
case SNDRV_PCM_IOCTL_SYNC_PTR32:
```
This error comes from a commit c0513348a7b39 ('sound: core: Avoid using
timespec for struct snd_pcm_sync_ptr').
Your patchset brought conflicts to 'for-next' branch in a repository
which Iwai-san maintains[1]. I rebased your patchset on a commit
729fbfc92a45 ('ALSA: line6: add support for POD HD DESKTOP') which is a
HEAD of 'for-next' branch and pushed into my repository on github[2].
I respect your work for this issue, however it's better to check whether
your patchset is buildable or not on major configurations before
posting.
I note that at a development period for v4.5 kernel, ALSA developers
(mainly Iwai-san) fixed x32 ABI compatibility bugs. Then I prepared for
a rough set of test for ioctl command[3] to check his work. The set will
partly help your work, I think (but it's really rough).
I need more time for reviewing. At least, this week is for recovery from
my tough work to rewrite aplay[4].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
[2] https://github.com/takaswie/sound/tree/topic/year2038-rfc1
[3] https://github.com/takaswie/alsa-ioctl-test/
[4] [alsa-devel] [RFCv2][PATCH 00/38] alsa-utils: axfer: rewrite aplay
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-September/125574.html
Thanks
Takashi Sakamoto
Re: [PATCH 23/25 v3] ALSA/dummy: Replace tasklet with softirq hrtimer
On Sep 6 2017 01:18, Sebastian Andrzej Siewior wrote:
From: Thomas Gleixner <t...@linutronix.de>
The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
callback in softirq context as well which renders the tasklet useless.
Signed-off-by: Thomas Gleixner <t...@linutronix.de>
Signed-off-by: Anna-Maria Gleixner <anna-ma...@linutronix.de>
Cc: Jaroslav Kysela <pe...@perex.cz>
Cc: Takashi Iwai <ti...@suse.com>
Cc: Takashi Sakamoto <o-taka...@sakamocchi.jp>
Cc: alsa-de...@alsa-project.org
[o-takashi: avoid stall due to a call of hrtimer_cancel() on a callback
of hrtimer]
Signed-off-by: Takashi Sakamoto <o-taka...@sakamocchi.jp>
Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
---
On 2017-09-06 01:05:43 [+0900], Takashi Sakamoto wrote:
As Iwai-san mentioned, in this function, .trigger can be called in two
cases; XRUN occurs and draining is done. Thus, let you change the comment as
'In cases of XRUN and draining, this calls .trigger to stop PCM substream.'.
I'm sorry to trouble you.
snd_pcm_period_elapsed()
->snd_pcm_update_hw_ptr0()
->snd_pcm_update_state()
->snd_pcm_drain_done()
...
->.trigger(TRIGGER_STOP)
->xrun()
->snd_pcm_stop()
...
->.trigger(TRIGGER_STOP)
I think you asked me just to update the comment. Did I do it right?
v2…v3: updated the comment as per Takashi Sakamoto's suggestion.
v1…v2: merged Takashi Sakamoto fixup of the original patch into v2.
sound/drivers/dummy.c | 25 +++--
1 file changed, 11 insertions(+), 14 deletions(-)
This Looks good to me. I did quick test and confirmed that it brings no
stalls.
Tested-by: Takashi Sakamoto <o-taka...@sakamocchi.jp>
diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index dd5ed037adf2..cdd851286f92 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -376,17 +376,9 @@ struct dummy_hrtimer_pcm {
ktime_t period_time;
atomic_t running;
struct hrtimer timer;
- struct tasklet_struct tasklet;
struct snd_pcm_substream *substream;
};
-static void dummy_hrtimer_pcm_elapsed(unsigned long priv)
-{
- struct dummy_hrtimer_pcm *dpcm = (struct dummy_hrtimer_pcm *)priv;
- if (atomic_read(>running))
- snd_pcm_period_elapsed(dpcm->substream);
-}
-
static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
{
struct dummy_hrtimer_pcm *dpcm;
@@ -394,7 +386,14 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct
hrtimer *timer)
dpcm = container_of(timer, struct dummy_hrtimer_pcm, timer);
if (!atomic_read(>running))
return HRTIMER_NORESTART;
- tasklet_schedule(>tasklet);
+ /*
+* In cases of XRUN and draining, this calls .trigger to stop PCM
+* substream.
+*/
+ snd_pcm_period_elapsed(dpcm->substream);
+ if (!atomic_read(>running))
+ return HRTIMER_NORESTART;
+
hrtimer_forward_now(timer, dpcm->period_time);
return HRTIMER_RESTART;
}
@@ -414,14 +413,14 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream
*substream)
struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
atomic_set(>running, 0);
- hrtimer_cancel(>timer);
+ if (!hrtimer_callback_running(>timer))
+ hrtimer_cancel(>timer);
return 0;
}
static inline void dummy_hrtimer_sync(struct dummy_hrtimer_pcm *dpcm)
{
hrtimer_cancel(>timer);
- tasklet_kill(>tasklet);
}
static snd_pcm_uframes_t
@@ -466,12 +465,10 @@ static int dummy_hrtimer_create(struct snd_pcm_substream
*substream)
if (!dpcm)
return -ENOMEM;
substream->runtime->private_data = dpcm;
- hrtimer_init(>timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+ hrtimer_init(>timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
dpcm->timer.function = dummy_hrtimer_callback;
dpcm->substream = substream;
atomic_set(>running, 0);
- tasklet_init(>tasklet, dummy_hrtimer_pcm_elapsed,
-(unsigned long)dpcm);
return 0;
}
Thanks
Takashi Sakamoto
