IPC 20190402 #772

lyakh · 2019-04-02T09:02:02Z

This removes mailbox reading from generic code and moves it down to Intel-specific implementations.

plbossart

@lyakh I did not get the idea behind your PR, really missed what ipc_read(), ipc_read_pcm_params() are supposed to represent and how they relate to actual IPC messages. Also the part on pcm_open/close leaves me scratching my head, and I still see a number of references to stream_box, etc, which make me think maybe there are still a number of assumptions related to mailboxes.

plbossart · 2019-04-02T15:33:59Z

sound/soc/sof/intel/Kconfig

+	help
+          This option is not user-selectable but automagically handled by
+	  'select' statements at a higher level
+


this isn't dependent on ACPI so should be moved to a common section.

plbossart · 2019-04-02T15:43:06Z

sound/soc/sof/intel/Kconfig

@@ -91,6 +99,7 @@ config SND_SOC_SOF_BROADWELL_SUPPORT
 config SND_SOC_SOF_BROADWELL
 	tristate
 	select SND_SOC_SOF_INTEL_COMMON
+	select SND_SOC_SOF_INTEL_COMMON_IPC


it's odd that SND_SOC_SOF_INTEL_COMMON_IPC is only used for legacy platforms before SKL. Probably needs to be renamed.

Yes, that's for non-HDA platforms, I'm open for better names :-)

SND_SOC_SOF_INTEL_HIFI_EP_IPC?

Ok, btw, what does EP there stand for? Also, it resembles SND_SOC_SOF_INTEL_ATOM_HIFI_EP but doesn't have ATOM in it and SND_SOC_SOF_INTEL_ATOM_HIFI_EP isn't used for Baytrail, whereas this new option is used for it too.

HIFI_EP is the name of the Tensilica core used before SKL.

sound/soc/sof/intel/byt.c

sound/soc/sof/intel/hda.h

plbossart · 2019-04-02T15:51:05Z

sound/soc/sof/ipc.c

 	struct snd_sof_pcm *spcm;
-	u32 posn_offset;
 	int direction;

 	/* check if we have stream box */
 	if (sdev->stream_box.size == 0) {


isn't this also completely Intel-specific and dependent on mailboxes?

Sure, they have to be removed too. This patch-set doesn't remove all Intel-specific code or objects from SOF, it's the next step in that direction.

plbossart · 2019-04-02T15:52:46Z

sound/soc/sof/ipc.c

 	int direction;

 	/* check if we have stream box */
 	if (sdev->stream_box.size == 0) {
 		/* read back full message */
-		snd_sof_dsp_mailbox_read(sdev, sdev->dsp_box.offset, &posn,
-					 sizeof(posn));
+		snd_sof_ipc_read(sdev, &posn, sizeof(posn));


I am lost as well on the definition of snd_sof_ipc_read. In one case you use it to read the data part of a RX message, but for trace and period elapsed it's not clear there is a RX message in the first place.

snd_sof_ipc_read() is a hw-neutral replacement for snd_sof_dsp_mailbox_read(). So, it is used everywhere where snd_sof_dsp_mailbox_read() was used before.

plbossart · 2019-04-02T15:54:44Z

sound/soc/sof/ops.h

+		return sof_ops(sdev)->ipc_read_pcm_params(sdev, substream,
+							  reply);
+
+	return 0;


isn't this mandatory and is 0 a relevant return value?

I'm not sure, if you think it should be mandatory, we can enforce it, sure

lyakh · 2019-04-02T16:31:39Z

@plbossart as a general reply: essentially this series eliminates the use of snd_sof_dsp_mailbox_read() in the SOF driver core. Let me add 2 more commits with more hw-abstracting to it.

plbossart · 2019-04-02T17:02:58Z

snd_sof_ipc_read() is a hw-neutral replacement for snd_sof_dsp_mailbox_read(). So, it is used everywhere where snd_sof_dsp_mailbox_read() was used before.

What I don't get is how this relates to actual IPC messages? on a non-Intel platform, what would this ipc_read() provide? access to the reply of a previous message? would it trigger a new IPC message at the lower-level? It looks like a renaming to me, I don't see how it became hw-neutral - probably missing the gist of the PR.

…

ranj063 · 2019-04-02T22:40:58Z

sound/soc/sof/intel/intel.c

+ * Intel-specific SOF code
+ */
+
+#include <linux/device.h>


@lyakh intel.c seems very generic. Can this name be a bit more specific? intel-ipc.c or intel-mailbox.c?

for now this file only contains IPC / mailbox code, but in principle it's thought as a place for any Intel SOF code. But if we want separate files, we can name this one more specifically.

sound/soc/sof/intel/skl.c

lyakh · 2019-04-03T11:35:22Z

snd_sof_ipc_read() is a hw-neutral replacement for snd_sof_dsp_mailbox_read(). So, it is used everywhere where snd_sof_dsp_mailbox_read() was used before.

What I don't get is how this relates to actual IPC messages? on a non-Intel platform, what would this ipc_read() provide? access to the reply of a previous message? would it trigger a new IPC message at the lower-level? It looks like a renaming to me, I don't see how it became hw-neutral - probably missing the gist of the PR.

@plbossart I could in principle port Sue Creek on top of this API to illustrate what those calls would be e.g. in an SPI IPC implementation. The current Sue Creek implementation does exactly that, yes, copies data from a temporary buffer.

In principle, yes, this PR does the renaming and some refactoring, eliminating Intel-specific arguments from generic functions. E.g. I find this

@@ -343,7 +343,7 @@ void snd_sof_ipc_msgs_rx(struct snd_sof_dev *sdev)
 	int err = 0;
 
 	/* read back header */
-	snd_sof_dsp_mailbox_read(sdev, sdev->dsp_box.offset, &hdr, sizeof(hdr));
+	snd_sof_ipc_read(sdev, &hdr, sizeof(hdr));
 	ipc_log_header(sdev->dev, "ipc rx", hdr.cmd);
 
 	cmd = hdr.cmd & SOF_GLB_TYPE_MASK;

less Intel-specific. It removes "mailbox" from the function name, which can be irrelevant for non-Intel implementations, and removes a mailbox from the function arguments.

plbossart · 2019-04-03T15:14:17Z

The current Sue Creek implementation does exactly that, yes, copies data from a temporary buffer.

temporary buffer filled when by whom?

When we use an explicit IPC, the core calls sof_ipc_tx_message() and gets a reply.
When the core calls snd_sof_ipc_read(), what is the expectation on what values are being read? Is this reading from the buffered reply? is this reading from a platform-specific location, and is there any agreement on what the format of the data might be?
It's really a description of the core/platform protocol I am asking for.

lyakh · 2019-04-03T16:13:05Z

The current Sue Creek implementation does exactly that, yes, copies data from a temporary buffer.

temporary buffer filled when by whom?

@plbossart By the interrupt thread when a DSP message triggers an interrupt.

When we use an explicit IPC, the core calls sof_ipc_tx_message() and gets a reply.
When the core calls snd_sof_ipc_read(), what is the expectation on what values are being read? Is this reading from the buffered reply? is this reading from a platform-specific location, and is there any agreement on what the format of the data might be?
It's really a description of the core/platform protocol I am asking for.

I see. Here's my understanding which still isn't perfect of course. On Intel platforms snd_sof_ipc_read() (the former snd_sof_dsp_mailbox_read()) reads from a mailbox where data has been placed by the DSP. Therefore these calls are only valid for the current message, before it gets overwritten. On non-Intel platforms the functionality has to be the same, but the implementation is up to the hardware. In cases similar to SPI, where the interface itself cannot store the data for random reading, the data can be stored in a temporary buffer in the interrupt thread.

plbossart · 2019-04-03T16:24:17Z

@lyakh so would this flow be correct:

sof_ipc_tx_message() triggers an exchange of information with the DSP.
The reply is extracted and provided back to the core
to get additional information not provided in the reply, the core needs to get more data with a call to ipc_read().

lyakh · 2019-04-03T16:33:11Z

@plbossart yes, or also snd_sof_ipc_read_pcm_stream() if it's per-stream data like an elapsed period or an xrun

plbossart · 2019-04-03T19:37:18Z

@plbossart yes, or also snd_sof_ipc_read_pcm_stream() if it's per-stream data like an elapsed period or an xrun

Ah ok, so it's not really a read() in the file IO/Linux sense but more a get_last_ipc_msg_data() helper function, where the data is typically cached in an intermediate buffer. Likewise it's get_last_ipc_stream_position(). I might be splitting hair here but I was thrown off by the "read" with no matching write.

lyakh · 2019-04-03T20:03:48Z

@plbossart yes, or also snd_sof_ipc_read_pcm_stream() if it's per-stream data like an elapsed period or an xrun

Ah ok, so it's not really a read() in the file IO/Linux sense but more a get_last_ipc_msg_data() helper function, where the data is typically cached in an intermediate buffer. Likewise it's get_last_ipc_stream_position(). I might be splitting hair here but I was thrown off by the "read" with no matching write.

No hair splitting at all! Naming and API improvement suggestions are appreciated! But, you aren't really proposing snd_sof_ipc_get_last_msg_data() and snd_sof_ipc_get_last_stream_position(), are you? ;-) How about snd_sof_ipc_msg_data() and snd_sof_ipc_stream_position? In fact, those names describe what those functions are currently used for. But actually they are more generic. E.g. the latter can be used for any stream-related data, not only position. If we consciously restrict it to position, we can modify its prototype to take struct sof_ipc_stream_posn * as an argument instead of a void * buffer and a size. In fact, on the other hand we could just have one function and set stream to NULL to get generic message data. I'm open to ideas.

lyakh · 2019-04-04T14:46:32Z

@plbossart btw, if we indeed make just one function with an optional stream parameter, then this

	if (sdev->stream_box.size)
		/* have stream box read from stream box */
		snd_sof_ipc_read_pcm_stream(sdev, stream->substream,
					    &posn, sizeof(posn));
	else
		/* read back full message */
		snd_sof_ipc_read(sdev, &posn, sizeof(posn));

will become just one call

plbossart · 2019-04-04T15:25:02Z

@plbossart btw, if we indeed make just one function with an optional stream parameter, then this
	if (sdev->stream_box.size)
		/* have stream box read from stream box */
		snd_sof_ipc_read_pcm_stream(sdev, stream->substream,
					    &posn, sizeof(posn));
	else
		/* read back full message */
		snd_sof_ipc_read(sdev, &posn, sizeof(posn));
will become just one call

I didn't fully get the last proposal, how would the last example be rewritten then?

lyakh · 2019-04-04T15:36:25Z

@plbossart btw, if we indeed make just one function with an optional stream parameter, then this
	if (sdev->stream_box.size)
		/* have stream box read from stream box */
		snd_sof_ipc_read_pcm_stream(sdev, stream->substream,
					    &posn, sizeof(posn));
	else
		/* read back full message */
		snd_sof_ipc_read(sdev, &posn, sizeof(posn));
will become just one call
I didn't fully get the last proposal, how would the last example be rewritten then?

Something like

	stream = sdev->stream_box.size ? stream->substream : NULL;
	snd_sof_ipc_read_pcm_stream(sdev, stream, &posn, sizeof(posn));

plbossart · 2019-04-04T16:11:56Z

Could we go one step further and only do:
snd_sof_ipc_read_pcm_stream(sdev, stream, &posn, sizeof(posn));
with the stream_box information only known to the platform parts. The core doesn't need to know anything about a stream_box.

lyakh · 2019-04-04T18:21:49Z

Could we go one step further and only do:
snd_sof_ipc_read_pcm_stream(sdev, stream, &posn, sizeof(posn));
with the stream_box information only known to the platform parts. The core doesn't need to know anything about a stream_box.

@plbossart Do you want to keep both calls: snd_sof_ipc_read() and snd_sof_ipc_read_pcm_stream() or only have one of them? We could have only one if we do

void snd_sof_ipc_read_pcm_stream(sdev, stream, buf, size)
{
	if (!stream || !sdev->stream_box.size)
		snd_sof_ipc_read(sdev, buf, size);
	else
		the_real_snd_sof_ipc_read_pcm_stream(sdev, stream, buf, size);
}

Is that what you'd like to have? With these names (obviously, not "the_real_") or with different ones?

plbossart · 2019-04-04T18:26:48Z

On 4/4/19 1:21 PM, Guennadi Liakhovetski wrote: Could we go one step further and only do: snd_sof_ipc_read_pcm_stream(sdev, stream, &posn, sizeof(posn)); with the stream_box information only known to the platform parts. The core doesn't need to know anything about a stream_box. @plbossart <https://github.com/plbossart> Do you want to keep both calls: snd_sof_ipc_read() and snd_sof_ipc_read_pcm_stream() or only have one of them? We could have only one if we do |void snd_sof_ipc_read_pcm_stream(sdev, stream, buf, size) { if (!stream || !sdev->stream_box.size) snd_sof_ipc_read(sdev, buf, size); else the_real_snd_sof_ipc_read_pcm_stream(sdev, stream, buf, size); } | Is that what you'd like to have? With these names (obviously, not "the_real_") or with different ones?

I was thinking of not doing any tests in the core, and let the platform-specific implementation figure out how it wants to get the data. This "if (!stream || !sdev->stream_box.size)" looks Intel-specific still.

lyakh · 2019-04-04T20:33:08Z

On 4/4/19 1:21 PM, Guennadi Liakhovetski wrote: Could we go one step further and only do: snd_sof_ipc_read_pcm_stream(sdev, stream, &posn, sizeof(posn)); with the stream_box information only known to the platform parts. The core doesn't need to know anything about a stream_box. @plbossart https://github.com/plbossart Do you want to keep both calls: snd_sof_ipc_read() and snd_sof_ipc_read_pcm_stream() or only have one of them? We could have only one if we do |void snd_sof_ipc_read_pcm_stream(sdev, stream, buf, size) { if (!stream || !sdev->stream_box.size) snd_sof_ipc_read(sdev, buf, size); else the_real_snd_sof_ipc_read_pcm_stream(sdev, stream, buf, size); } | Is that what you'd like to have? With these names (obviously, not "the_real_") or with different ones?
I was thinking of not doing any tests in the core, and let the platform-specific implementation figure out how it wants to get the data. This "if (!stream || !sdev->stream_box.size)" looks Intel-specific still.

Yes, sure, sorry, It would go into the Intel implementation, I didn't abstract it properly in that pseudo-code. I.e. it would go into hda_ipc_read_pcm_stream() and intel_ipc_read_pcm_stream(). BTW, do you prefer to keep the new file as intel.c for all the Intel-specific code or intel-ipc.c?

plbossart · 2019-04-04T20:40:48Z

Yes, sure, sorry, It would go into the Intel implementation, I didn't abstract it properly in that pseudo-code. I.e. it would go into hda_ipc_read_pcm_stream() and intel_ipc_read_pcm_stream(). BTW, do you prefer to keep the new file as intel.c for all the Intel-specific code or intel-ipc.c?

intel-ipc.c seems more logical and would prevent this file from becoming the dumping ground of whatever we come up with.
I am good with the renames and repartitioning you suggested, looking forward to an update soonish, I'd really like to have this as part of the v6 upstream batch. Thanks!

lyakh · 2019-04-05T14:04:43Z

@plbossart I ended up with

void intel_ipc_msg_data(struct snd_sof_dev *sdev,
			struct snd_pcm_substream *substream,
			void *p, size_t sz);
int intel_ipc_pcm_params(struct snd_sof_dev *sdev,
			 struct snd_pcm_substream *substream,
			 const struct sof_ipc_pcm_params_reply *reply);

plbossart

Thanks @lyakh. Looks very good, I only have a couple of nitpicks below. Since you will be changing the Kconfig, please rebase once I merged the global update today. Also checkpatch report a number of alignment issues that don't seem necessary, please try and fix them.
Have a nice week-end!

plbossart · 2019-04-05T15:38:38Z

sound/soc/sof/intel/Kconfig

@@ -91,6 +99,7 @@ config SND_SOC_SOF_BROADWELL_SUPPORT
 config SND_SOC_SOF_BROADWELL
 	tristate
 	select SND_SOC_SOF_INTEL_COMMON
+	select SND_SOC_SOF_INTEL_COMMON_IPC


SND_SOC_SOF_INTEL_HIFI_EP_IPC?

plbossart · 2019-04-05T15:40:14Z

sound/soc/sof/intel/hda-stream.c

-		stream = devm_kzalloc(sdev->dev, sizeof(*stream), GFP_KERNEL);
-		if (!stream)
+		struct sof_intel_hda_stream *hda_stream = devm_kzalloc(sdev->dev,
+					sizeof(*hda_stream), GFP_KERNEL);


maybe the variable declaration on one line, and the devm_kzcalloc assignment on the next to make things more readable?

readability is somewhat subjective, but if that's your preference... :-) But in general, I think I'll fall back to this pattern time and time again. I usually prefer

int ret = f(x); /* optional empty line, which I personally consider more obstructing than useful */ if (ret < 0) return ret;

over

int ret; ret = f(x); if (ret < 0) return ret;

or

struct x_s *x = get_x(y);

over

struct x_s *x x = get_x(y);

and also

struct x_s *x = get_x(y); struct z_s *z = get_z(x);

instead of 4 lines. I find compact code easier to read, when you get the whole block in front of your eyes on the screen, instead of having to paginate. Of course, one instruction per line remains, so, things like

if (x) break;

aren't helpful any more. But well, it's all a matter of personal preference and habit, I guess.

I tend to prefer one variable per line and avoid mixing declarations and assignments with a function call. Some coding standards actually don't allow any initialization in the declaration block btw.

I'm sure there are all possible coding standards out there, whereas we're following the kernel standard here ;-) Then come ALSA, ASoC and your preferences, as long as downstream standards don't contradict the upstream ones. I also think it's good not to be too creative downstream and not impose too many additional restrictions on top of Documentation/process/coding-style.rst

...I think what makes sense is this: when a new file is created its author is free to impose their micro-style on it within the coding-style.rst (and, possibly, any further styling preferences of the respective subsystem, but it's good to keep those to the minimum). Then when an existing file is edited, it's a very good idea to follow that file's micro-style. In this case in hda-stream.c I see code like

struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata; struct sof_intel_dsp_bdl *bdl; int i, offset, period_bytes, periods;

which both contains initialisations and multiple variable declarations per line. So, I think it should be ok to keep that style.

sound/soc/sof/intel/hda.h

sound/soc/sof/intel/intel-ipc.c

sound/soc/sof/sof-priv.h

plbossart · 2019-04-08T21:56:57Z

- stream = devm_kzalloc(sdev->dev, sizeof(*stream), GFP_KERNEL); - if (!stream) + struct sof_intel_hda_stream *hda_stream = devm_kzalloc(sdev->dev, + sizeof(*hda_stream), GFP_KERNEL); ...I think what makes sense is this: when a new file is created its author is free to impose their micro-style on it /within/ the coding-style.rst (and, possibly, any further styling preferences of the

you're breaking your own rule here. checkpatch.pl --strict complains about parenthesis alignment...

lyakh · 2019-04-09T06:13:32Z

you're breaking your own rule here. checkpatch.pl --strict complains about parenthesis alignment...

@plbossart From Documentation/process/5.Posting.rst

You should always
run patches through scripts/checkpatch.pl and address the complaints it
comes up with. Please bear in mind that checkpatch.pl, while being the
embodiment of a fair amount of thought about what kernel patches should
look like, is not smarter than you. If fixing a checkpatch.pl complaint
would make the code worse, don't do it.

From Documentation/process/submit-checklist.rst

Check for trivial violations with the patch style checker prior to
submission (scripts/checkpatch.pl).
You should be able to justify all violations that remain in
your patch.

plbossart

the code is fine but please fix alignment issues witch checkpatch.pl --strict.

plbossart · 2019-04-09T14:31:59Z

sound/soc/sof/intel/hda-stream.c

@@ -676,6 +684,7 @@ void hda_dsp_stream_free(struct snd_sof_dev *sdev)
 			snd_dma_free_pages(&s->bdl);
 		list_del(&s->list);
 		stream = stream_to_hdac_ext_stream(s);
-		devm_kfree(sdev->dev, stream);
+		devm_kfree(sdev->dev, container_of(stream,
+				struct sof_intel_hda_stream, hda_stream));


plbossart · 2019-04-09T14:33:29Z

sound/soc/sof/ops.h

+/* host configure DSP HW parameters */
+static inline int snd_sof_ipc_pcm_params(struct snd_sof_dev *sdev,
+				struct snd_pcm_substream *substream,
+				const struct sof_ipc_pcm_params_reply *reply)


A recent commit f3adfd6 ("ASoC:sof: remove duplicate posn message in kernel log")' removed a redundant logging entry in ipc_period_elapsed() but it missed a similar ont in ipc_xrun(). Remove the latter one too. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Even if the stream position IPC from the DSP isn't using the stream mailbox, there is no need to read the complete message to get a message ID, it must be the same as the one, obtained earlier from the same mailbox. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Obtaining data from the DSP by reading the mailbox is Intel-specific, move them from the SOF core to Intel-specific drivers. Signed-off-by: Guennadi Liakhovetski <[email protected]>

IPC implementation using mailboxes is hardware-specific, no need to export it globally. Signed-off-by: Guennadi Liakhovetski <[email protected]>

IPC implementation using mailboxes is hardware-specific and it is meaningless on serial busses like SPI. Signed-off-by: Guennadi Liakhovetski <[email protected]>

IPC implementation using mailboxes is hardware-specific, no need to export it globally. Signed-off-by: Guennadi Liakhovetski <[email protected]>

snd_sof_fw_parse_ext_data() is always called for the MMIO BAR, remove the argument and specify the BAR explicitly. Signed-off-by: Guennadi Liakhovetski <[email protected]>

.block_read() and .block_write() operations as well as their respective implementations sof_block_read() and sof_block_write() are always used with the MMIO BAR. Remove the argument and specify the BAR explicitly. Signed-off-by: Guennadi Liakhovetski <[email protected]>

lyakh · 2019-04-09T15:17:50Z

the code is fine but please fix alignment issues witch checkpatch.pl --strict.

@plbossart hmm, I looked through a couple of files, and indeed - the parentheses alignment is strictly observed everywhere! Indeed it would be a pity to break it now :-) But you don't really mean that SOF pedantically removes every single remark produced by checkpatch --strict?.. You said yourself a while ago, that you even would accept 80+ in some cases. Do we have a list of which (of at least the most frequent) checkpatch complaints we remove unconditionally and which are considered less critical? E.g. I now have (following the style in ops.h)

static inline int
snd_sof_ipc_pcm_params(struct snd_sof_dev *sdev,
		       struct snd_pcm_substream *substream,
		       const struct sof_ipc_pcm_params_reply *reply)

which I personally don't like, but that's the way the 80+ / alignment problem is solved there... An update is on its way.

plbossart · 2019-04-09T15:32:58Z

the code is fine but please fix alignment issues witch checkpatch.pl --strict.

@plbossart hmm, I looked through a couple of files, and indeed - the parentheses alignment is strictly observed everywhere! Indeed it would be a pity to break it now :-) But you don't really mean that SOF pedantically removes every single remark produced by checkpatch --strict?.. You said yourself a while ago, that you even would accept 80+ in some cases. Do we have a list of which (of at least the most frequent) checkpatch complaints we remove unconditionally and which are considered less critical? E.g. I now have (following the style in ops.h)
static inline int
snd_sof_ipc_pcm_params(struct snd_sof_dev *sdev,
		       struct snd_pcm_substream *substream,
		       const struct sof_ipc_pcm_params_reply *reply)
which I personally don't like, but that's the way the 80+ / alignment problem is solved there... An update is on its way.

I really like everything aligned, otherwise we introduce spurious changes if someone else uses a different alignment in the future.
The 80 char rule is not a strict one for me, I don't really care if it's violated to keep the code readable.
Same for the rule where people add a new line after a for loop, it's fine.

plbossart · 2019-04-09T17:05:40Z

@xiulipan same CI issue, the code was pushed 2 hours ago.
@wenqingfu can you try and restart the CI directly? We'll lose one entire day at this rate.

plbossart · 2019-04-09T17:28:29Z

merging this one as well after checking that the code compiles on my machine. Gah.

…do() dev->unlink_list is reused unless dev is deleted. So, list_del() should not be used. Due to using list_del(), dev->unlink_list can't be reused so that dev->nested_level update logic doesn't work. In order to fix this bug, list_del_init() should be used instead of list_del(). Test commands: ip link add bond0 type bond ip link add bond1 type bond ip link set bond0 master bond1 ip link set bond0 nomaster ip link set bond1 master bond0 ip link set bond1 nomaster Splat looks like: [ 255.750458][ T1030] ============================================ [ 255.751967][ T1030] WARNING: possible recursive locking detected [ 255.753435][ T1030] 5.9.0-rc8+ thesofproject#772 Not tainted [ 255.754553][ T1030] -------------------------------------------- [ 255.756047][ T1030] ip/1030 is trying to acquire lock: [ 255.757304][ T1030] ffff88811782a280 (&dev_addr_list_lock_key/1){+...}-{2:2}, at: dev_mc_sync_multiple+0xc2/0x150 [ 255.760056][ T1030] [ 255.760056][ T1030] but task is already holding lock: [ 255.761862][ T1030] ffff88811130a280 (&dev_addr_list_lock_key/1){+...}-{2:2}, at: bond_enslave+0x3d4d/0x43e0 [bonding] [ 255.764581][ T1030] [ 255.764581][ T1030] other info that might help us debug this: [ 255.766645][ T1030] Possible unsafe locking scenario: [ 255.766645][ T1030] [ 255.768566][ T1030] CPU0 [ 255.769415][ T1030] ---- [ 255.770259][ T1030] lock(&dev_addr_list_lock_key/1); [ 255.771629][ T1030] lock(&dev_addr_list_lock_key/1); [ 255.772994][ T1030] [ 255.772994][ T1030] *** DEADLOCK *** [ 255.772994][ T1030] [ 255.775091][ T1030] May be due to missing lock nesting notation [ 255.775091][ T1030] [ 255.777182][ T1030] 2 locks held by ip/1030: [ 255.778299][ T1030] #0: ffffffffb1f63250 (rtnl_mutex){+.+.}-{3:3}, at: rtnetlink_rcv_msg+0x2e4/0x8b0 [ 255.780600][ T1030] #1: ffff88811130a280 (&dev_addr_list_lock_key/1){+...}-{2:2}, at: bond_enslave+0x3d4d/0x43e0 [bonding] [ 255.783411][ T1030] [ 255.783411][ T1030] stack backtrace: [ 255.784874][ T1030] CPU: 7 PID: 1030 Comm: ip Not tainted 5.9.0-rc8+ thesofproject#772 [ 255.786595][ T1030] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014 [ 255.789030][ T1030] Call Trace: [ 255.789850][ T1030] dump_stack+0x99/0xd0 [ 255.790882][ T1030] __lock_acquire.cold.71+0x166/0x3cc [ 255.792285][ T1030] ? register_lock_class+0x1a30/0x1a30 [ 255.793619][ T1030] ? rcu_read_lock_sched_held+0x91/0xc0 [ 255.794963][ T1030] ? rcu_read_lock_bh_held+0xa0/0xa0 [ 255.796246][ T1030] lock_acquire+0x1b8/0x850 [ 255.797332][ T1030] ? dev_mc_sync_multiple+0xc2/0x150 [ 255.798624][ T1030] ? bond_enslave+0x3d4d/0x43e0 [bonding] [ 255.800039][ T1030] ? check_flags+0x50/0x50 [ 255.801143][ T1030] ? lock_contended+0xd80/0xd80 [ 255.802341][ T1030] _raw_spin_lock_nested+0x2e/0x70 [ 255.803592][ T1030] ? dev_mc_sync_multiple+0xc2/0x150 [ 255.804897][ T1030] dev_mc_sync_multiple+0xc2/0x150 [ 255.806168][ T1030] bond_enslave+0x3d58/0x43e0 [bonding] [ 255.807542][ T1030] ? __lock_acquire+0xe53/0x51b0 [ 255.808824][ T1030] ? bond_update_slave_arr+0xdc0/0xdc0 [bonding] [ 255.810451][ T1030] ? check_chain_key+0x236/0x5e0 [ 255.811742][ T1030] ? mutex_is_locked+0x13/0x50 [ 255.812910][ T1030] ? rtnl_is_locked+0x11/0x20 [ 255.814061][ T1030] ? netdev_master_upper_dev_get+0xf/0x120 [ 255.815553][ T1030] do_setlink+0x94c/0x3040 [ ... ] Reported-by: [email protected] Fixes: 1fc70ed ("net: core: add nested_level variable in net_device") Signed-off-by: Taehee Yoo <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jakub Kicinski <[email protected]>

plbossart reviewed Apr 2, 2019

View reviewed changes

ranj063 reviewed Apr 2, 2019

View reviewed changes

sound/soc/sof/intel/skl.c Show resolved Hide resolved

lyakh force-pushed the ipc-20190402 branch from 48f1c50 to c78e8df Compare April 5, 2019 14:03

plbossart requested changes Apr 5, 2019

View reviewed changes

lyakh force-pushed the ipc-20190402 branch from c78e8df to fb9c474 Compare April 8, 2019 09:28

plbossart reviewed Apr 9, 2019

View reviewed changes

lyakh added 3 commits April 9, 2019 17:00

ASoC: SOF: IPC: remove direct mailbox reading from SOF core

5baa5a7

Obtaining data from the DSP by reading the mailbox is Intel-specific, move them from the SOF core to Intel-specific drivers. Signed-off-by: Guennadi Liakhovetski <[email protected]>

lyakh added 8 commits April 9, 2019 17:12

ASoC: SOF: skl: remove direct mailbox reading from SOF core

e92feaa

Obtaining data from the DSP by reading the mailbox is Intel-specific, move them from the SOF core to Intel-specific drivers. Signed-off-by: Guennadi Liakhovetski <[email protected]>

ASoC: SOF: hsw: remove direct mailbox reading from SOF core

f0ed859

Obtaining data from the DSP by reading the mailbox is Intel-specific, move them from the SOF core to Intel-specific drivers. Signed-off-by: Guennadi Liakhovetski <[email protected]>

ASoC: SOF: remove .mailbox_{read,write}() operations

6af46a4

IPC implementation using mailboxes is hardware-specific, no need to export it globally. Signed-off-by: Guennadi Liakhovetski <[email protected]>

ASoC: SOF: spi: remove .mailbox_{read,write}() operations

36b6830

IPC implementation using mailboxes is hardware-specific and it is meaningless on serial busses like SPI. Signed-off-by: Guennadi Liakhovetski <[email protected]>

ASoC: SOF: hsw: remove .mailbox_{read,write}() operations

734eba3

IPC implementation using mailboxes is hardware-specific, no need to export it globally. Signed-off-by: Guennadi Liakhovetski <[email protected]>

ASoC: SOF: skl: remove .mailbox_{read,write}() operations

cfee6b7

IPC implementation using mailboxes is hardware-specific, no need to export it globally. Signed-off-by: Guennadi Liakhovetski <[email protected]>

ASoC: SOF: IPC: remove a superfluous function argument

71f992c

snd_sof_fw_parse_ext_data() is always called for the MMIO BAR, remove the argument and specify the BAR explicitly. Signed-off-by: Guennadi Liakhovetski <[email protected]>

lyakh force-pushed the ipc-20190402 branch from fb9c474 to e044b43 Compare April 9, 2019 15:27

plbossart merged commit a79122d into thesofproject:topic/sof-dev Apr 9, 2019

lyakh deleted the ipc-20190402 branch April 10, 2019 06:02

dbaluta mentioned this pull request Apr 10, 2019

Revert commits removing bar parameter #811

Merged

IPC 20190402 #772

IPC 20190402 #772

Conversation

lyakh commented Apr 2, 2019

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyakh commented Apr 2, 2019

plbossart commented Apr 2, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyakh commented Apr 3, 2019 • edited Loading

plbossart commented Apr 3, 2019

lyakh commented Apr 3, 2019

plbossart commented Apr 3, 2019

lyakh commented Apr 3, 2019

plbossart commented Apr 3, 2019

lyakh commented Apr 3, 2019

lyakh commented Apr 4, 2019

plbossart commented Apr 4, 2019

lyakh commented Apr 4, 2019

plbossart commented Apr 4, 2019

lyakh commented Apr 4, 2019

plbossart commented Apr 4, 2019 via email

lyakh commented Apr 4, 2019

plbossart commented Apr 4, 2019

lyakh commented Apr 5, 2019

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart commented Apr 8, 2019 via email

lyakh commented Apr 9, 2019

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyakh commented Apr 9, 2019

plbossart commented Apr 9, 2019

plbossart commented Apr 9, 2019

plbossart commented Apr 9, 2019

lyakh commented Apr 3, 2019 •

edited

Loading