[PATCH v2 0/4] soundwire: better error handling in deferred transfers
This is a generic cleanup series.
v2: - Remove the "soundwire: stream: uniquify dev_err() logs" patch
Pierre-Louis Bossart (4): soundwire: stream: use consistent pattern for freeing buffers soundwire: bus: remove sdw_defer argument in sdw_transfer_defer() soundwire: cadence: use directly bus sdw_defer structure soundwire: cadence: further simplify low-level xfer_msg_defer() callback
drivers/soundwire/bus.c | 12 +++++------- drivers/soundwire/bus.h | 3 +-- drivers/soundwire/cadence_master.c | 20 ++++++++++---------- drivers/soundwire/cadence_master.h | 5 +---- drivers/soundwire/stream.c | 8 ++++---- include/linux/soundwire/sdw.h | 6 +++--- 6 files changed, 24 insertions(+), 30 deletions(-)
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
The code should free the message buffer used for data, the message structure used for control and assign the latter to NULL. The last part is missing for multi-link cases, and the order is inconsistent for single-link cases.
Link: https://github.com/thesofproject/linux/issues/4056 Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/stream.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index df3b36670df4..9c13dbd2b26e 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -723,8 +723,8 @@ static int sdw_bank_switch(struct sdw_bus *bus, int m_rt_count) }
if (!multi_link) { - kfree(wr_msg); kfree(wbuf); + kfree(wr_msg); bus->defer_msg.msg = NULL; bus->params.curr_bank = !bus->params.curr_bank; bus->params.next_bank = !bus->params.next_bank; @@ -769,6 +769,7 @@ static int sdw_ml_sync_bank_switch(struct sdw_bus *bus) if (bus->defer_msg.msg) { kfree(bus->defer_msg.msg->buf); kfree(bus->defer_msg.msg); + bus->defer_msg.msg = NULL; }
return 0; @@ -867,6 +868,7 @@ static int do_bank_switch(struct sdw_stream_runtime *stream) if (bus->defer_msg.msg) { kfree(bus->defer_msg.msg->buf); kfree(bus->defer_msg.msg); + bus->defer_msg.msg = NULL; } }
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
There's no point in passing an argument that is a pointer to a bus member. We can directly get the member and do an indirection when needed.
This is a first step before simplifying the hardware-specific callbacks further.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/bus.c | 10 ++++------ drivers/soundwire/bus.h | 3 +-- drivers/soundwire/stream.c | 4 +--- 3 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 633d411b64f3..572cc9a9af41 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -225,9 +225,9 @@ static inline int do_transfer(struct sdw_bus *bus, struct sdw_msg *msg) }
static inline int do_transfer_defer(struct sdw_bus *bus, - struct sdw_msg *msg, - struct sdw_defer *defer) + struct sdw_msg *msg) { + struct sdw_defer *defer = &bus->defer_msg; int retry = bus->prop.err_threshold; enum sdw_command_response resp; int ret = 0, i; @@ -335,19 +335,17 @@ EXPORT_SYMBOL(sdw_show_ping_status); * sdw_transfer_defer() - Asynchronously transfer message to a SDW Slave device * @bus: SDW bus * @msg: SDW message to be xfered - * @defer: Defer block for signal completion * * Caller needs to hold the msg_lock lock while calling this */ -int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg, - struct sdw_defer *defer) +int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg) { int ret;
if (!bus->ops->xfer_msg_defer) return -ENOTSUPP;
- ret = do_transfer_defer(bus, msg, defer); + ret = do_transfer_defer(bus, msg); if (ret != 0 && ret != -ENODATA) dev_err(bus->dev, "Defer trf on Slave %d failed:%d\n", msg->dev_num, ret); diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h index 7631ef5e71fb..96927a143796 100644 --- a/drivers/soundwire/bus.h +++ b/drivers/soundwire/bus.h @@ -151,8 +151,7 @@ int sdw_configure_dpn_intr(struct sdw_slave *slave, int port, bool enable, int mask);
int sdw_transfer(struct sdw_bus *bus, struct sdw_msg *msg); -int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg, - struct sdw_defer *defer); +int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg);
#define SDW_READ_INTR_CLEAR_RETRY 10
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 9c13dbd2b26e..2e39587ed1de 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -684,8 +684,6 @@ static int sdw_bank_switch(struct sdw_bus *bus, int m_rt_count) if (!wr_msg) return -ENOMEM;
- bus->defer_msg.msg = wr_msg; - wbuf = kzalloc(sizeof(*wbuf), GFP_KERNEL); if (!wbuf) { ret = -ENOMEM; @@ -713,7 +711,7 @@ static int sdw_bank_switch(struct sdw_bus *bus, int m_rt_count) multi_link = bus->multi_link && (m_rt_count >= bus->hw_sync_min_links);
if (multi_link) - ret = sdw_transfer_defer(bus, wr_msg, &bus->defer_msg); + ret = sdw_transfer_defer(bus, wr_msg); else ret = sdw_transfer(bus, wr_msg);
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Copying the bus sdw_defer structure into the Cadence internals leads to using stale pointers and kernel oopses on errors. It's just simpler and safer to use the bus sdw_defer structure directly.
Link: https://github.com/thesofproject/linux/issues/4056 Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/bus.c | 2 +- drivers/soundwire/cadence_master.c | 17 ++++++++--------- drivers/soundwire/cadence_master.h | 5 +---- include/linux/soundwire/sdw.h | 3 +-- 4 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 572cc9a9af41..fc192d0c61e7 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -237,7 +237,7 @@ static inline int do_transfer_defer(struct sdw_bus *bus, init_completion(&defer->complete);
for (i = 0; i <= retry; i++) { - resp = bus->ops->xfer_msg_defer(bus, msg, defer); + resp = bus->ops->xfer_msg_defer(bus, msg); ret = find_response_code(resp); /* if cmd is ok or ignored return */ if (ret == 0 || ret == -ENODATA) diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 521387322145..2365395cb181 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -750,7 +750,7 @@ EXPORT_SYMBOL(cdns_xfer_msg);
enum sdw_command_response cdns_xfer_msg_defer(struct sdw_bus *bus, - struct sdw_msg *msg, struct sdw_defer *defer) + struct sdw_msg *msg) { struct sdw_cdns *cdns = bus_to_cdns(bus); int cmd = 0, ret; @@ -763,9 +763,6 @@ cdns_xfer_msg_defer(struct sdw_bus *bus, if (ret) return SDW_CMD_FAIL_OTHER;
- cdns->defer = defer; - cdns->defer->length = msg->len; - return _cdns_xfer_msg(cdns, msg, cmd, 0, msg->len, true); } EXPORT_SYMBOL(cdns_xfer_msg_defer); @@ -893,13 +890,15 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id) return IRQ_NONE;
if (int_status & CDNS_MCP_INT_RX_WL) { + struct sdw_bus *bus = &cdns->bus; + struct sdw_defer *defer = &bus->defer_msg; + cdns_read_response(cdns);
- if (cdns->defer) { - cdns_fill_msg_resp(cdns, cdns->defer->msg, - cdns->defer->length, 0); - complete(&cdns->defer->complete); - cdns->defer = NULL; + if (defer && defer->msg) { + cdns_fill_msg_resp(cdns, defer->msg, + defer->length, 0); + complete(&defer->complete); } else { complete(&cdns->tx_complete); } diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index fa9dc38264a4..53029d22822d 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -109,7 +109,6 @@ struct sdw_cdns_dai_runtime { * @instance: instance number * @response_buf: SoundWire response buffer * @tx_complete: Tx completion - * @defer: Defer pointer * @ports: Data ports * @num_ports: Total number of data ports * @pcm: PCM streams @@ -130,7 +129,6 @@ struct sdw_cdns { u32 response_buf[CDNS_MCP_IP_MAX_CMD_LEN + 2];
struct completion tx_complete; - struct sdw_defer *defer;
struct sdw_cdns_port *ports; int num_ports; @@ -189,8 +187,7 @@ enum sdw_command_response cdns_xfer_msg(struct sdw_bus *bus, struct sdw_msg *msg);
enum sdw_command_response -cdns_xfer_msg_defer(struct sdw_bus *bus, - struct sdw_msg *msg, struct sdw_defer *defer); +cdns_xfer_msg_defer(struct sdw_bus *bus, struct sdw_msg *msg);
u32 cdns_read_ping_status(struct sdw_bus *bus);
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 3cd2a761911f..fb2bd1524a26 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -852,8 +852,7 @@ struct sdw_master_ops { enum sdw_command_response (*xfer_msg) (struct sdw_bus *bus, struct sdw_msg *msg); enum sdw_command_response (*xfer_msg_defer) - (struct sdw_bus *bus, struct sdw_msg *msg, - struct sdw_defer *defer); + (struct sdw_bus *bus, struct sdw_msg *msg); enum sdw_command_response (*reset_page_addr) (struct sdw_bus *bus, unsigned int dev_num); int (*set_bus_conf)(struct sdw_bus *bus,
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
The message pointer is already stored in the bus->defer structure, not need to pass it as an argument.
Suggested-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/bus.c | 2 +- drivers/soundwire/cadence_master.c | 5 +++-- drivers/soundwire/cadence_master.h | 2 +- include/linux/soundwire/sdw.h | 5 +++-- 4 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index fc192d0c61e7..ed94c92bc575 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -237,7 +237,7 @@ static inline int do_transfer_defer(struct sdw_bus *bus, init_completion(&defer->complete);
for (i = 0; i <= retry; i++) { - resp = bus->ops->xfer_msg_defer(bus, msg); + resp = bus->ops->xfer_msg_defer(bus); ret = find_response_code(resp); /* if cmd is ok or ignored return */ if (ret == 0 || ret == -ENODATA) diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 2365395cb181..ece0ad89746f 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -749,10 +749,11 @@ cdns_xfer_msg(struct sdw_bus *bus, struct sdw_msg *msg) EXPORT_SYMBOL(cdns_xfer_msg);
enum sdw_command_response -cdns_xfer_msg_defer(struct sdw_bus *bus, - struct sdw_msg *msg) +cdns_xfer_msg_defer(struct sdw_bus *bus) { struct sdw_cdns *cdns = bus_to_cdns(bus); + struct sdw_defer *defer = &bus->defer_msg; + struct sdw_msg *msg = defer->msg; int cmd = 0, ret;
/* for defer only 1 message is supported */ diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index 53029d22822d..63c58b9b1f59 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -187,7 +187,7 @@ enum sdw_command_response cdns_xfer_msg(struct sdw_bus *bus, struct sdw_msg *msg);
enum sdw_command_response -cdns_xfer_msg_defer(struct sdw_bus *bus, struct sdw_msg *msg); +cdns_xfer_msg_defer(struct sdw_bus *bus);
u32 cdns_read_ping_status(struct sdw_bus *bus);
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index fb2bd1524a26..86e320cf27b1 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -837,7 +837,8 @@ struct sdw_defer { * @read_prop: Read Master properties * @override_adr: Override value read from firmware (quirk for buggy firmware) * @xfer_msg: Transfer message callback - * @xfer_msg_defer: Defer version of transfer message callback + * @xfer_msg_defer: Defer version of transfer message callback. The message is handled with the + * bus struct @sdw_defer * @reset_page_addr: Reset the SCP page address registers * @set_bus_conf: Set the bus configuration * @pre_bank_switch: Callback for pre bank switch @@ -852,7 +853,7 @@ struct sdw_master_ops { enum sdw_command_response (*xfer_msg) (struct sdw_bus *bus, struct sdw_msg *msg); enum sdw_command_response (*xfer_msg_defer) - (struct sdw_bus *bus, struct sdw_msg *msg); + (struct sdw_bus *bus); enum sdw_command_response (*reset_page_addr) (struct sdw_bus *bus, unsigned int dev_num); int (*set_bus_conf)(struct sdw_bus *bus,
participants (2)
-
Bard Liao -
Vinod Koul