All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ALSA: dice: improve support for ancient firmware for DICE
@ 2018-04-22 12:07 Takashi Sakamoto
  2018-04-22 18:25 ` kbuild test robot
  0 siblings, 1 reply; 5+ messages in thread
From: Takashi Sakamoto @ 2018-04-22 12:07 UTC (permalink / raw)
  To: clemens, tiwai; +Cc: alsa-devel, ffado-devel

In early stage of firmware SDK, DICE seems to lose its backward
compatibility due to some registers on global address section. I found
this with Alesis Multimix 12 FireWire with ancient firmware (approx.
shipped version).

According to retrieved log from the unit, global section has 96 byte
space. On the other hand, current version of ALSA dice driver assumes
that all of supported unit has at least 100 byte space.

$ ./firewire-request /dev/fw1 read 0xffffe0000000 28
result: 000: 00 00 00 0a 00 00 00 18 00 00 00 22 00 00 00 8a
result: 010: 00 00 00 ac 00 00 01 12 00 00 00 00 00 00 00 00
result: 020: 00 00 00 00 00 00 00 00

This commit adds support for the ancient firmware. Check of global section
is loosened to accept the smaller space. The lack of information is
already compensated by hard-coded parameters.

I experienced that the latest version of Windows driver for this model
can't handle this unit, too. This means that TCAT releases firmware SDK
without backward compatibility for the ancient firmware.

Below list is a early history of driver/firmware package released by
Alesis. I investigated on wayback machine on Internet Archive:
 * Unknown: PAL v1.0.41.2, firmware v1.0.3
 * Mar 2006: PAL v1.54.0, firmware v1.0.4
 * Dec 2006: PAL v2.0.0.2, firmware v2.0
 * Jun 2007: PAL v3.0.41.5, firmware v2.0
 * Jul 2007: PAL v3.0.56.2. firmware v2.0
 * Jan 2008: PAL v3.0.81.1080, firmware v2.0

If I can assume that firmware version is the same as DICE version,
DICE version for the issued firmware may be v1.0.3. According to code
base of userspace driver project (FFADO), I can read DICE v1.0.4 supports
global space larger than 100 byte. I guess the smaller space of global
section is a feature of DICE v1.0.3 but no way to clear it.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/dice/dice-interface.h   |  9 +++++--
 sound/firewire/dice/dice-proc.c        | 10 +++----
 sound/firewire/dice/dice-transaction.c | 49 +++++++++++++++++++---------------
 3 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/sound/firewire/dice/dice-interface.h b/sound/firewire/dice/dice-interface.h
index 15a484b05298..9cad3d608229 100644
--- a/sound/firewire/dice/dice-interface.h
+++ b/sound/firewire/dice/dice-interface.h
@@ -174,14 +174,19 @@
  */
 #define GLOBAL_SAMPLE_RATE		0x05c
 
+/*
+ * Some old firmware versions do not have the following global registers.
+ * Windows drivers produced by TCAT lost backward compatibility in its
+ * early release because they can handle firmware only which supports the
+ * following registers.
+ */
+
 /*
  * The version of the DICE driver specification that this device conforms to;
  * read-only.
  */
 #define GLOBAL_VERSION			0x060
 
-/* Some old firmware versions do not have the following global registers: */
-
 /*
  * Supported sample rates and clock sources; read-only.
  */
diff --git a/sound/firewire/dice/dice-proc.c b/sound/firewire/dice/dice-proc.c
index f5c1d1bced59..cc079323ed30 100644
--- a/sound/firewire/dice/dice-proc.c
+++ b/sound/firewire/dice/dice-proc.c
@@ -148,12 +148,12 @@ static void dice_proc_read(struct snd_info_entry *entry,
 				   >> CLOCK_RATE_SHIFT));
 	snd_iprintf(buffer, "  ext status: %08x\n", buf.global.extended_status);
 	snd_iprintf(buffer, "  sample rate: %u\n", buf.global.sample_rate);
-	snd_iprintf(buffer, "  version: %u.%u.%u.%u\n",
-		    (buf.global.version >> 24) & 0xff,
-		    (buf.global.version >> 16) & 0xff,
-		    (buf.global.version >>  8) & 0xff,
-		    (buf.global.version >>  0) & 0xff);
 	if (quadlets >= 90) {
+		snd_iprintf(buffer, "  version: %u.%u.%u.%u\n",
+			    (buf.global.version >> 24) & 0xff,
+			    (buf.global.version >> 16) & 0xff,
+			    (buf.global.version >>  8) & 0xff,
+			    (buf.global.version >>  0) & 0xff);
 		snd_iprintf(buffer, "  clock caps:");
 		for (i = 0; i <= 6; ++i)
 			if (buf.global.clock_caps & (1 << i))
diff --git a/sound/firewire/dice/dice-transaction.c b/sound/firewire/dice/dice-transaction.c
index 0f0350320ae8..2410c082bf8e 100644
--- a/sound/firewire/dice/dice-transaction.c
+++ b/sound/firewire/dice/dice-transaction.c
@@ -265,7 +265,7 @@ int snd_dice_transaction_reinit(struct snd_dice *dice)
 static int get_subaddrs(struct snd_dice *dice)
 {
 	static const int min_values[10] = {
-		10, 0x64 / 4,
+		10, 0x60 / 4,
 		10, 0x18 / 4,
 		10, 0x18 / 4,
 		0, 0,
@@ -301,33 +301,40 @@ static int get_subaddrs(struct snd_dice *dice)
 		}
 	}
 
-	/*
-	 * Check that the implemented DICE driver specification major version
-	 * number matches.
-	 */
-	err = snd_fw_transaction(dice->unit, TCODE_READ_QUADLET_REQUEST,
-				 DICE_PRIVATE_SPACE +
-				 be32_to_cpu(pointers[0]) * 4 + GLOBAL_VERSION,
-				 &version, sizeof(version), 0);
-	if (err < 0)
-		goto end;
+	if (be32_to_cpu(pointers[1]) > 0x18) {
+		/*
+		 * Check that the implemented DICE driver specification major
+		 * version number matches.
+		 */
+		err = snd_fw_transaction(dice->unit, TCODE_READ_QUADLET_REQUEST,
+				DICE_PRIVATE_SPACE +
+				be32_to_cpu(pointers[0]) * 4 + GLOBAL_VERSION,
+				&version, sizeof(version), 0);
+		if (err < 0)
+			goto end;
 
-	if ((version & cpu_to_be32(0xff000000)) != cpu_to_be32(0x01000000)) {
-		dev_err(&dice->unit->device,
-			"unknown DICE version: 0x%08x\n", be32_to_cpu(version));
-		err = -ENODEV;
-		goto end;
+		if ((version & cpu_to_be32(0xff000000)) !=
+						cpu_to_be32(0x01000000)) {
+			dev_err(&dice->unit->device,
+				"unknown DICE version: 0x%08x\n",
+				be32_to_cpu(version));
+			err = -ENODEV;
+			goto end;
+		}
+
+		/* Set up later. */
+		dice->clock_caps = 1;
 	}
 
 	dice->global_offset = be32_to_cpu(pointers[0]) * 4;
 	dice->tx_offset = be32_to_cpu(pointers[2]) * 4;
 	dice->rx_offset = be32_to_cpu(pointers[4]) * 4;
-	dice->sync_offset = be32_to_cpu(pointers[6]) * 4;
-	dice->rsrv_offset = be32_to_cpu(pointers[8]) * 4;
 
-	/* Set up later. */
-	if (be32_to_cpu(pointers[1]) * 4 >= GLOBAL_CLOCK_CAPABILITIES + 4)
-		dice->clock_caps = 1;
+	/* Old firmware doesn't support these fields. */
+	if (pointers[7] > 0)
+		dice->sync_offset = be32_to_cpu(pointers[6]) * 4;
+	if (pointers[9] > 0)
+		dice->rsrv_offset = be32_to_cpu(pointers[8]) * 4;
 end:
 	kfree(pointers);
 	return err;
-- 
2.14.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] ALSA: dice: improve support for ancient firmware for DICE
  2018-04-22 12:07 [PATCH] ALSA: dice: improve support for ancient firmware for DICE Takashi Sakamoto
@ 2018-04-22 18:25 ` kbuild test robot
  2018-04-24 11:49   ` Takashi Iwai
  0 siblings, 1 reply; 5+ messages in thread
From: kbuild test robot @ 2018-04-22 18:25 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: tiwai, alsa-devel, clemens, kbuild-all, ffado-devel

Hi Takashi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on sound/for-next]
[also build test WARNING on v4.17-rc1 next-20180420]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Takashi-Sakamoto/ALSA-dice-improve-support-for-ancient-firmware-for-DICE/20180422-230429
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> sound/firewire/dice/dice-transaction.c:334:21: sparse: restricted __be32 degrades to integer
   sound/firewire/dice/dice-transaction.c:336:21: sparse: restricted __be32 degrades to integer

vim +334 sound/firewire/dice/dice-transaction.c

   264	
   265	static int get_subaddrs(struct snd_dice *dice)
   266	{
   267		static const int min_values[10] = {
   268			10, 0x60 / 4,
   269			10, 0x18 / 4,
   270			10, 0x18 / 4,
   271			0, 0,
   272			0, 0,
   273		};
   274		__be32 *pointers;
   275		__be32 version;
   276		u32 data;
   277		unsigned int i;
   278		int err;
   279	
   280		pointers = kmalloc_array(ARRAY_SIZE(min_values), sizeof(__be32),
   281					 GFP_KERNEL);
   282		if (pointers == NULL)
   283			return -ENOMEM;
   284	
   285		/*
   286		 * Check that the sub address spaces exist and are located inside the
   287		 * private address space.  The minimum values are chosen so that all
   288		 * minimally required registers are included.
   289		 */
   290		err = snd_fw_transaction(dice->unit, TCODE_READ_BLOCK_REQUEST,
   291					 DICE_PRIVATE_SPACE, pointers,
   292					 sizeof(__be32) * ARRAY_SIZE(min_values), 0);
   293		if (err < 0)
   294			goto end;
   295	
   296		for (i = 0; i < ARRAY_SIZE(min_values); ++i) {
   297			data = be32_to_cpu(pointers[i]);
   298			if (data < min_values[i] || data >= 0x40000) {
   299				err = -ENODEV;
   300				goto end;
   301			}
   302		}
   303	
   304		if (be32_to_cpu(pointers[1]) > 0x18) {
   305			/*
   306			 * Check that the implemented DICE driver specification major
   307			 * version number matches.
   308			 */
   309			err = snd_fw_transaction(dice->unit, TCODE_READ_QUADLET_REQUEST,
   310					DICE_PRIVATE_SPACE +
   311					be32_to_cpu(pointers[0]) * 4 + GLOBAL_VERSION,
   312					&version, sizeof(version), 0);
   313			if (err < 0)
   314				goto end;
   315	
   316			if ((version & cpu_to_be32(0xff000000)) !=
   317							cpu_to_be32(0x01000000)) {
   318				dev_err(&dice->unit->device,
   319					"unknown DICE version: 0x%08x\n",
   320					be32_to_cpu(version));
   321				err = -ENODEV;
   322				goto end;
   323			}
   324	
   325			/* Set up later. */
   326			dice->clock_caps = 1;
   327		}
   328	
   329		dice->global_offset = be32_to_cpu(pointers[0]) * 4;
   330		dice->tx_offset = be32_to_cpu(pointers[2]) * 4;
   331		dice->rx_offset = be32_to_cpu(pointers[4]) * 4;
   332	
   333		/* Old firmware doesn't support these fields. */
 > 334		if (pointers[7] > 0)
   335			dice->sync_offset = be32_to_cpu(pointers[6]) * 4;
   336		if (pointers[9] > 0)
   337			dice->rsrv_offset = be32_to_cpu(pointers[8]) * 4;
   338	end:
   339		kfree(pointers);
   340		return err;
   341	}
   342	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ALSA: dice: improve support for ancient firmware for DICE
  2018-04-22 18:25 ` kbuild test robot
@ 2018-04-24 11:49   ` Takashi Iwai
  2018-04-24 12:57     ` Takashi Sakamoto
  0 siblings, 1 reply; 5+ messages in thread
From: Takashi Iwai @ 2018-04-24 11:49 UTC (permalink / raw)
  To: Takashi Sakamoto
  Cc: alsa-devel, clemens, kbuild test robot, ffado-devel, kbuild-all

On Sun, 22 Apr 2018 20:25:36 +0200,
kbuild test robot wrote:
> 
> sparse warnings: (new ones prefixed by >>)
> 
> >> sound/firewire/dice/dice-transaction.c:334:21: sparse: restricted __be32 degrades to integer
>    sound/firewire/dice/dice-transaction.c:336:21: sparse: restricted __be32 degrades to integer
> 
> vim +334 sound/firewire/dice/dice-transaction.c
(snip)
>    333		/* Old firmware doesn't support these fields. */
>  > 334		if (pointers[7] > 0)
>    335			dice->sync_offset = be32_to_cpu(pointers[6]) * 4;
>    336		if (pointers[9] > 0)
>    337			dice->rsrv_offset = be32_to_cpu(pointers[8]) * 4;

The warning looks reasonable.
Alternatively, you can rewrite it as a non-zero check:
	if (pointers[7])
		....
Then it would be OK without endian conversion.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ALSA: dice: improve support for ancient firmware for DICE
  2018-04-24 11:49   ` Takashi Iwai
@ 2018-04-24 12:57     ` Takashi Sakamoto
  2018-04-24 13:35       ` Takashi Sakamoto
  0 siblings, 1 reply; 5+ messages in thread
From: Takashi Sakamoto @ 2018-04-24 12:57 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: alsa-devel, clemens, kbuild test robot, ffado-devel, kbuild-all

Hi,

On Apr 24 2018 20:49, Takashi Iwai wrote:
> On Sun, 22 Apr 2018 20:25:36 +0200,
> kbuild test robot wrote:
>>
>> sparse warnings: (new ones prefixed by >>)
>>
>>>> sound/firewire/dice/dice-transaction.c:334:21: sparse: restricted __be32 degrades to integer
>>     sound/firewire/dice/dice-transaction.c:336:21: sparse: restricted __be32 degrades to integer
>>
>> vim +334 sound/firewire/dice/dice-transaction.c
> (snip)
>>     333		/* Old firmware doesn't support these fields. */
>>   > 334		if (pointers[7] > 0)
>>     335			dice->sync_offset = be32_to_cpu(pointers[6]) * 4;
>>     336		if (pointers[9] > 0)
>>     337			dice->rsrv_offset = be32_to_cpu(pointers[8]) * 4;
> 
> The warning looks reasonable.

Yep. The integer value can be negative.

> Alternatively, you can rewrite it as a non-zero check:
> 	if (pointers[7])
> 		....
> Then it would be OK without endian conversion.

OK. I'll repost.


Thanks

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ALSA: dice: improve support for ancient firmware for DICE
  2018-04-24 12:57     ` Takashi Sakamoto
@ 2018-04-24 13:35       ` Takashi Sakamoto
  0 siblings, 0 replies; 5+ messages in thread
From: Takashi Sakamoto @ 2018-04-24 13:35 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: alsa-devel, clemens, kbuild test robot, ffado-devel, kbuild-all

On Apr 24 2018 21:57, Takashi Sakamoto wrote:
> Hi,
> 
> On Apr 24 2018 20:49, Takashi Iwai wrote:
>> On Sun, 22 Apr 2018 20:25:36 +0200,
>> kbuild test robot wrote:
>>>
>>> sparse warnings: (new ones prefixed by >>)
>>>
>>>>> sound/firewire/dice/dice-transaction.c:334:21: sparse: restricted 
>>>>> __be32 degrades to integer
>>>     sound/firewire/dice/dice-transaction.c:336:21: sparse: restricted 
>>> __be32 degrades to integer
>>>
>>> vim +334 sound/firewire/dice/dice-transaction.c
>> (snip)
>>>     333        /* Old firmware doesn't support these fields. */
>>>   > 334        if (pointers[7] > 0)
>>>     335            dice->sync_offset = be32_to_cpu(pointers[6]) * 4;
>>>     336        if (pointers[9] > 0)
>>>     337            dice->rsrv_offset = be32_to_cpu(pointers[8]) * 4;
>>
>> The warning looks reasonable.
> 
> Yep. The integer value can be negative.
> 
>> Alternatively, you can rewrite it as a non-zero check:
>>     if (pointers[7])
>>         ....
>> Then it would be OK without endian conversion.
> 
> OK. I'll repost.

I reposted it now and it's blasted[1]. Please evaluate and apply it 
instead of the one which I mistook to send.


[1] 
http://mailman.alsa-project.org/pipermail/alsa-devel/2018-April/134936.html

Thanks

Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-04-24 13:35 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-22 12:07 [PATCH] ALSA: dice: improve support for ancient firmware for DICE Takashi Sakamoto
2018-04-22 18:25 ` kbuild test robot
2018-04-24 11:49   ` Takashi Iwai
2018-04-24 12:57     ` Takashi Sakamoto
2018-04-24 13:35       ` Takashi Sakamoto

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.