| Message ID | 20221014210618.3012368-6-jan@3e8.eu |
|---|---|
| State | Changes Requested |
| Delegated to: | Sander Vanheule |
| Headers | show |
| Series | realtek: avoid blocking for too long | expand |
On Fri, 2022-10-14 at 23:06 +0200, Jan Hoffmann wrote: > These two functions are identical apart from writing different values to > the read/write bit. Create a new function rtl_table_exec to reduce code > duplication. > > Also replace the unbounded busy-waiting loop. As the hardware usually > responds very quickly, always call cond_resched, so that callers do not > need to worry about blocking for too long. This is especially important > for the L2 table, as a full loop (as in rtl83xx_port_fdb_dump) takes > about 20ms on RTL839x (and that function actually ends up being called > in a loop for every port). > > So far, polling timeout errors are only handled by logging an error, but > a return value is added to allow proper handling in the future. > > Signed-off-by: Jan Hoffmann <jan@3e8.eu> > --- > > I'm not really sure if putting cond_resched in this place is really the > best solution. An alternative would be to place it directly in the loops > (i.e. rtl83xx_port_fdb_dump and l2_table_show) instead. I'm no expert on this, but it does appear to be more common to call cond_resched() from loops. If rtl_table_exec() once is not an issue, but calling it repeatedly is, then it would make more sense to me to put this in the loops. If anyone want to argue otherwise, I'd be happy to hear so though. Otherwise this patch looks good to me. Putting upper bounds on loop durations, however unlikely to fail, is not something I'm going to refuse :) > > About error handling: Actually implementing that for all calls is going > to require large changes. And doing it in a proper way which is better > than just printing an error (i.e. actually trying to leave a consistent > state) is non-trivial. I started to work on that only for fdb/mdb > access, but that is only a part of where these tables are used: > > https://github.com/janh/openwrt/commit/e45403299208d49675a0403ad94349ebdff2a374 > https://github.com/janh/openwrt/commit/fa68b23cab542683832696f039cf160b96da83db > https://github.com/janh/openwrt/commit/92abfc2e4925e5bc1a0e25e464ca1d08833c9b30 Thanks for your work on improving this driver! Best, Sander
diff --git a/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c b/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c index 13e63a5f0c5d..5e7c65247fc0 100644 --- a/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c +++ b/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c @@ -121,28 +121,51 @@ void rtl_table_release(struct table_reg *r) // pr_info("Unlock done\n"); } +static int rtl_table_exec(struct table_reg *r, bool is_write, int idx) +{ + int ret = 0; + u32 cmd, val; + + /* Read/write bit has inverted meaning on RTL838x */ + if (r->rmode) + cmd = is_write ? 0 : BIT(r->c_bit); + else + cmd = is_write ? BIT(r->c_bit) : 0; + + cmd |= BIT(r->c_bit + 1); /* Execute bit */ + cmd |= r->tbl << r->t_bit; /* Table type */ + cmd |= idx & (BIT(r->t_bit) - 1); /* Index */ + + sw_w32(cmd, r->addr); + + /* The hardware typically responds immediately, so make sure that + * other tasks always get a chance to run even if this function is + * called in a loop. + */ + cond_resched(); + + ret = readx_poll_timeout(sw_r32, r->addr, val, + !(val & BIT(r->c_bit + 1)), 20, 10000); + if (ret) + pr_err("%s: timeout\n", __func__); + + return ret; +} + /* * Reads table index idx into the data registers of the table */ -void rtl_table_read(struct table_reg *r, int idx) +int rtl_table_read(struct table_reg *r, int idx) { - u32 cmd = r->rmode ? BIT(r->c_bit) : 0; - - cmd |= BIT(r->c_bit + 1) | (r->tbl << r->t_bit) | (idx & (BIT(r->t_bit) - 1)); - sw_w32(cmd, r->addr); - do { } while (sw_r32(r->addr) & BIT(r->c_bit + 1)); + return rtl_table_exec(r, false, idx); } /* * Writes the content of the table data registers into the table at index idx */ -void rtl_table_write(struct table_reg *r, int idx) +int rtl_table_write(struct table_reg *r, int idx) { - u32 cmd = r->rmode ? 0 : BIT(r->c_bit); - - cmd |= BIT(r->c_bit + 1) | (r->tbl << r->t_bit) | (idx & (BIT(r->t_bit) - 1)); - sw_w32(cmd, r->addr); - do { } while (sw_r32(r->addr) & BIT(r->c_bit + 1)); + return rtl_table_exec(r, true, idx); } /* diff --git a/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/rtl83xx.h b/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/rtl83xx.h index 107016469c69..485d0e8a7e9c 100644 --- a/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/rtl83xx.h +++ b/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/rtl83xx.h @@ -67,8 +67,8 @@ typedef enum { void rtl_table_init(void); struct table_reg *rtl_table_get(rtl838x_tbl_reg_t r, int t); void rtl_table_release(struct table_reg *r); -void rtl_table_read(struct table_reg *r, int idx); -void rtl_table_write(struct table_reg *r, int idx); +int rtl_table_read(struct table_reg *r, int idx); +int rtl_table_write(struct table_reg *r, int idx); inline u16 rtl_table_data(struct table_reg *r, int i); inline u32 rtl_table_data_r(struct table_reg *r, int i); inline void rtl_table_data_w(struct table_reg *r, u32 v, int i);
These two functions are identical apart from writing different values to the read/write bit. Create a new function rtl_table_exec to reduce code duplication. Also replace the unbounded busy-waiting loop. As the hardware usually responds very quickly, always call cond_resched, so that callers do not need to worry about blocking for too long. This is especially important for the L2 table, as a full loop (as in rtl83xx_port_fdb_dump) takes about 20ms on RTL839x (and that function actually ends up being called in a loop for every port). So far, polling timeout errors are only handled by logging an error, but a return value is added to allow proper handling in the future. Signed-off-by: Jan Hoffmann <jan@3e8.eu> --- I'm not really sure if putting cond_resched in this place is really the best solution. An alternative would be to place it directly in the loops (i.e. rtl83xx_port_fdb_dump and l2_table_show) instead. About error handling: Actually implementing that for all calls is going to require large changes. And doing it in a proper way which is better than just printing an error (i.e. actually trying to leave a consistent state) is non-trivial. I started to work on that only for fdb/mdb access, but that is only a part of where these tables are used: https://github.com/janh/openwrt/commit/e45403299208d49675a0403ad94349ebdff2a374 https://github.com/janh/openwrt/commit/fa68b23cab542683832696f039cf160b96da83db https://github.com/janh/openwrt/commit/92abfc2e4925e5bc1a0e25e464ca1d08833c9b30 .../drivers/net/dsa/rtl83xx/common.c | 47 ++++++++++++++----- .../drivers/net/dsa/rtl83xx/rtl83xx.h | 4 +- 2 files changed, 37 insertions(+), 14 deletions(-)