Skip to content

Leave SoundWire IRQs enabled during device removal #5264

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
charleskeepax wants to merge 2 commits into from
Closed

Leave SoundWire IRQs enabled during device removal #5264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
38c6604
ASoC: Intel: sof_sdw: Add space for a terminator into DAIs array
charleskeepax Dec 2, 2024
7a3019a
soundwire: intel_auxdevice: Don't disable IRQs before removing children
charleskeepax Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions drivers/soundwire/intel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ struct sdw_intel_link_res {
u32 link_mask;
struct sdw_cdns *cdns;
struct list_head list;
struct mutex *link_lock; /* lock protecting list */
struct hdac_bus *hbus;
};

Expand Down
5 changes: 4 additions & 1 deletion drivers/soundwire/intel_auxdevice.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,9 +477,12 @@ static void intel_link_remove(struct auxiliary_device *auxdev)
if (!bus->prop.hw_disabled) {
sdw_intel_debugfs_exit(sdw);
cancel_delayed_work_sync(&cdns->attach_dwork);
sdw_cdns_enable_interrupt(cdns, false);
}

sdw_bus_master_delete(bus);

if (!bus->prop.hw_disabled)
sdw_cdns_enable_interrupt(cdns, false);
}

int intel_link_process_wakeen_event(struct auxiliary_device *auxdev)
Expand Down
16 changes: 16 additions & 0 deletions drivers/soundwire/intel_init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,15 @@ static void intel_link_dev_release(struct device *dev)
kfree(ldev);
}

static void intel_link_list_del(void *data)
{
struct sdw_intel_link_res *link = data;

mutex_lock(link->link_lock);
list_del(&link->list);
mutex_unlock(link->link_lock);
}

/* alloc, init and add link devices */
static struct sdw_intel_link_dev *intel_link_dev_register(struct sdw_intel_res *res,
struct sdw_intel_ctx *ctx,
Expand Down Expand Up @@ -78,6 +87,7 @@ static struct sdw_intel_link_dev *intel_link_dev_register(struct sdw_intel_res *
link->shim_vs = res->mmio_base + SDW_SHIM2_VS_BASE(link_id);
link->shim_lock = res->eml_lock;
}
link->link_lock = &ctx->link_lock;

link->ops = res->ops;
link->dev = res->dev;
Expand Down Expand Up @@ -144,8 +154,10 @@ irqreturn_t sdw_intel_thread(int irq, void *dev_id)
struct sdw_intel_ctx *ctx = dev_id;
struct sdw_intel_link_res *link;

mutex_lock(&ctx->link_lock);
list_for_each_entry(link, &ctx->link_list, list)
sdw_cdns_irq(irq, link->cdns);
mutex_unlock(&ctx->link_lock);

return IRQ_HANDLED;
}
Expand Down Expand Up @@ -209,6 +221,7 @@ static struct sdw_intel_ctx
ctx->link_mask = res->link_mask;
ctx->handle = res->handle;
mutex_init(&ctx->shim_lock);
mutex_init(&ctx->link_lock);

link_mask = ctx->link_mask;

Expand Down Expand Up @@ -245,7 +258,10 @@ static struct sdw_intel_ctx
i++;
goto err;
}
mutex_lock(&ctx->link_lock);
list_add_tail(&link->list, &ctx->link_list);
mutex_unlock(&ctx->link_lock);
devm_add_action_or_reset(&ldev->auxdev.dev, intel_link_list_del, link);
bus = &link->cdns->bus;
Comment on lines +261 to 265
Copy link
Member

@lgirdwood lgirdwood Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lot of locking here to manage the in flight IRQs as we iterate the list. Have you tried to safely iterate/remove the list elems ?

Copy link
Author

@charleskeepax charleskeepax Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe one can safely traverse a list that will be accessed from multiple threads without locking. There are _safe versions of the list macros, but they are only safe against accesses from within your own thread, say if the loop over the list might also modify the list.

That said the actual contention on this lock should be close to zero, since the list is only modified during probe and remove. The rest of the time the only thing accessing the list is the IRQ handler.

Copy link
Member

@lgirdwood lgirdwood Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I wondered if you able to safely remove the device from the list before device->remove() and device->irq(), but it seems not in this case.

Copy link
Author

@charleskeepax charleskeepax Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trouble is the IRQ is part of communicating with the device, and one generally wants to communicate with the device whilst doing a remove to park the device in a sensible state. The simplest way of avoid a clash on the list is to mask the IRQ before the remove, which is infact what the code used to do. But this causes us problems as we can't communicate with the device anymore.

/* Calculate number of slaves */
list_for_each(node, &bus->slaves)
Expand Down
1 change: 1 addition & 0 deletions include/linux/soundwire/sdw_intel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,7 @@ struct sdw_intel_ctx {
acpi_handle handle;
struct sdw_intel_link_dev **ldev;
struct list_head link_list;
struct mutex link_lock; /* lock protecting link_list */
struct mutex shim_lock; /* lock for access to shared SHIM registers */
u32 shim_mask;
u32 shim_base;
Expand Down
8 changes: 6 additions & 2 deletions sound/soc/intel/boards/sof_sdw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1108,8 +1108,12 @@ static int sof_card_dai_links_create(struct snd_soc_card *card)
return ret;
}

/* One per DAI link, worst case is a DAI link for every endpoint */
sof_dais = kcalloc(num_ends, sizeof(*sof_dais), GFP_KERNEL);
/*
* One per DAI link, worst case is a DAI link for every endpoint, also
* add one additional to act as a terminator such that code can iterate
* until it hits an uninitialised DAI.
*/
sof_dais = kcalloc(num_ends + 1, sizeof(*sof_dais), GFP_KERNEL);
Copy link
Member

@lgirdwood lgirdwood Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not fix this at source i.e. where its being dereferenced ?

Copy link
Author

@charleskeepax charleskeepax Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial intention was to reduce the number of parameters getting passed around, I guess the alternative fix would be something like:

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index 7ed49416c1a4..98b17d1e82f2 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -893,7 +893,7 @@ static int create_sdw_dailink(struct snd_soc_card *card,
 
 static int create_sdw_dailinks(struct snd_soc_card *card,
                               struct snd_soc_dai_link **dai_links, int *be_id,
-                          struct asoc_sdw_dailink *sof_dais,
+                        struct asoc_sdw_dailink *sof_dais, int num_sof_dais,
                               struct snd_soc_codec_conf **codec_conf)
 {
        struct asoc_sdw_mc_private *ctx = snd_soc_card_get_drvdata(card);
@@ -904,7 +904,8 @@ static int create_sdw_dailinks(struct snd_soc_card *card,
                intel_ctx->sdw_pin_index[i] = SOC_SDW_INTEL_BIDIR_PDI_BASE;
 
        /* generate DAI links by each sdw link */
-   while (sof_dais->initialised) {
+ i = 0;
+ while (i++ < num_sof_dais && sof_dais->initialised) {
                int current_be_id;
 
                ret = create_sdw_dailink(card, sof_dais, dai_links,
@@ -1170,7 +1171,7 @@ static int sof_card_dai_links_create(struct snd_soc_card *card)
        /* SDW */
        if (sdw_be_num) {
                ret = create_sdw_dailinks(card, &dai_links, &be_id,
-                                     sof_dais, &codec_conf);
+                                   sof_dais, num_ends, &codec_conf);
                if (ret)
                        goto err_end;
        }

Happy to go with that if we prefer, although personally I slightly prefer the approach of just having a spare sof_dais entry.

Copy link
Collaborator

@bardliao bardliao Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this version with a comment to explain why we need the additional sof_dai

Copy link
Member

@lgirdwood lgirdwood Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, best to fix at source with a comment otherwise new clients could make the same mistake.

if (!sof_dais)
return -ENOMEM;

Expand Down
Loading