On Wed, Dec 22, 2021 at 10:54:41AM +0100, Lukas Bulwahn wrote:
> Dear Vaibhav, dear Johan, dear Alex, dear Greg,
>
> I have seen that the greybus arche driver has been under heavy
> development in 2016 and 2017 with some further clean-up in 2019.
>
> However, so far, the config GREYBUS_ARCHE for this driver still
> depends on the out-of-tree config USB_HSIC_USB3613, with a proper
> exception made for compile testing (with COMPILE_TEST).
>
> Will this USB_HSIC_USB3613 config and driver still be added in the
> mainline kernel in the near future, so that the config dependencies
> are consistent in mainline?
Do you have this hardware? If so, we can add the driver, but given that
I did not think the chip ever actually shipped, it didn't make much
sense.
> Or, are the further out-of-tree additions still maintained for the
> current kernel and will stay out of tree? Is this arche driver not
> needed anymore and can be dropped?
Do you want to drop it as it is causing problems for you? It's a good
example driver for those wanting to create a greybus host controller
driver so it's nice to have in the tree, unless you have a different one
that should be merged instead?
thanks,
greg k-h
On Fri, Dec 17, 2021 at 11:34:08AM -0300, Jorge Eduardo Fermino Oliveia Silva wrote:
[Note: I am traveling for the next week so I won't be very responsive.]
Hi Jorge.
Before we get to the platch please remember that you should send all
Greybus patches to greybus-dev(a)lists.linaro.org and
linux-kernel(a)vger.kernel.org. I will add them in now and leave all of
the context so other can see what you sent.
> Solve CHECK: Lines should not end with a '('
>
> Reported-by: Jorge Eduardo Fermino Oliveia Silva <jorgeubermensch(a)gmail.com>
> Signed-off-by: Jorge Eduardo Fermino Oliveia Silva <jorgeubermensch(a)gmail.com>
> ---
> drivers/staging/greybus/audio_manager_private.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/greybus/audio_manager_private.h b/drivers/staging/greybus/audio_manager_private.h
> index 2b3a766c7de7..a17f09a19014 100644
> --- a/drivers/staging/greybus/audio_manager_private.h
> +++ b/drivers/staging/greybus/audio_manager_private.h
> @@ -12,10 +12,10 @@
>
> #include "audio_manager.h"
>
> -int gb_audio_manager_module_create(
> - struct gb_audio_manager_module **module,
> - struct kset *manager_kset,
> - int id, struct gb_audio_manager_module_descriptor *desc);
> +int gb_audio_manager_module_create(struct gb_audio_manager_module **module,
> + struct kset *manager_kset,
> + int id,
> + struct gb_audio_manager_module_descriptor *desc);
>
> /* module destroyed via kobject_put */
The part you're removing has all of the parameters at the same
indentation level and what you adding has them at two different
indentation levels so I'm not sure this is a step forward. Since the
kernel coding style doesn't address this specific case, AFAICS, I would
leave it as is despite the complaint. If others disagree then go ahead
as I really don't care much either way.
Mark
--
On 12/11/21 9:16 PM, Jason Wang wrote:
> The double `for' in the comment in line 81 is repeated. Remove one
> of them from the comment.
This looks fine. But it's so trivial... Are you aware
of *any* other similar trivial problems in comments that
could be fixed together with this? If so, I would prefer
that.
If you've looked, and there are no others:
Reviewed-by: Alex Elder <elder(a)linaro.org>
> Signed-off-by: Jason Wang <wangborong(a)cdjrlc.com>
> ---
> drivers/greybus/es2.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/greybus/es2.c b/drivers/greybus/es2.c
> index 15661c7f3633..e89cca015095 100644
> --- a/drivers/greybus/es2.c
> +++ b/drivers/greybus/es2.c
> @@ -78,7 +78,7 @@ struct es2_cport_in {
> * @hd: pointer to our gb_host_device structure
> *
> * @cport_in: endpoint, urbs and buffer for cport in messages
> - * @cport_out_endpoint: endpoint for for cport out messages
> + * @cport_out_endpoint: endpoint for cport out messages
> * @cport_out_urb: array of urbs for the CPort out messages
> * @cport_out_urb_busy: array of flags to see if the @cport_out_urb is busy or
> * not.
>
Convert greybus/uart.c from IDR to XArray. The abstract data type XArray
is more memory-efficient, parallelisable, and cache friendly. It takes
advantage of RCU to perform lookups without locking. Furthermore, IDR is
deprecated because XArray has a better (cleaner and more consistent) API.
Signed-off-by: Fabio M. De Francesco <fmdefrancesco(a)gmail.com>
---
v3->v4:
Remove mutex_lock/unlock around xa_load(). These locks seem to
be unnecessary because there is a 1:1 correspondence between
a specific minor and its gb_tty and there is no reference
counting. I think that the RCU locks used inside xa_load()
are sufficient to protect this API from returning an invalid
gb_tty in case of concurrent access. Some more considerations
on this topic are in the following message to linux-kernel list:
https://lore.kernel.org/lkml/3554184.2JXonMZcNW@localhost.localdomain/
v2->v3:
Fix some issues according to a review by Alex Elder <elder(a)ieee.org>
v1->v2:
Fix an issue found by the kernel test robot. It is due to
passing to xa_*lock() the same old mutex that IDR used with
the previous version of the code.
drivers/staging/greybus/uart.c | 32 ++++++++++++++------------------
1 file changed, 14 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
index 73f01ed1e5b7..f66983adb51b 100644
--- a/drivers/staging/greybus/uart.c
+++ b/drivers/staging/greybus/uart.c
@@ -22,7 +22,7 @@
#include <linux/serial.h>
#include <linux/tty_driver.h>
#include <linux/tty_flip.h>
-#include <linux/idr.h>
+#include <linux/xarray.h>
#include <linux/fs.h>
#include <linux/kdev_t.h>
#include <linux/kfifo.h>
@@ -32,8 +32,9 @@
#include "gbphy.h"
-#define GB_NUM_MINORS 16 /* 16 is more than enough */
-#define GB_NAME "ttyGB"
+#define GB_NUM_MINORS 16 /* 16 is more than enough */
+#define GB_RANGE_MINORS XA_LIMIT(0, GB_NUM_MINORS)
+#define GB_NAME "ttyGB"
#define GB_UART_WRITE_FIFO_SIZE PAGE_SIZE
#define GB_UART_WRITE_ROOM_MARGIN 1 /* leave some space in fifo */
@@ -67,8 +68,7 @@ struct gb_tty {
};
static struct tty_driver *gb_tty_driver;
-static DEFINE_IDR(tty_minors);
-static DEFINE_MUTEX(table_lock);
+static DEFINE_XARRAY(tty_minors);
static int gb_uart_receive_data_handler(struct gb_operation *op)
{
@@ -341,8 +341,7 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor)
{
struct gb_tty *gb_tty;
- mutex_lock(&table_lock);
- gb_tty = idr_find(&tty_minors, minor);
+ gb_tty = xa_load(&tty_minors, minor);
if (gb_tty) {
mutex_lock(&gb_tty->mutex);
if (gb_tty->disconnected) {
@@ -353,19 +352,18 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor)
mutex_unlock(&gb_tty->mutex);
}
}
- mutex_unlock(&table_lock);
return gb_tty;
}
static int alloc_minor(struct gb_tty *gb_tty)
{
int minor;
+ int ret;
- mutex_lock(&table_lock);
- minor = idr_alloc(&tty_minors, gb_tty, 0, GB_NUM_MINORS, GFP_KERNEL);
- mutex_unlock(&table_lock);
- if (minor >= 0)
- gb_tty->minor = minor;
+ ret = xa_alloc(&tty_minors, &minor, gb_tty, GB_RANGE_MINORS, GFP_KERNEL);
+ if (ret)
+ return ret;
+ gb_tty->minor = minor;
return minor;
}
@@ -374,9 +372,7 @@ static void release_minor(struct gb_tty *gb_tty)
int minor = gb_tty->minor;
gb_tty->minor = 0; /* Maybe should use an invalid value instead */
- mutex_lock(&table_lock);
- idr_remove(&tty_minors, minor);
- mutex_unlock(&table_lock);
+ xa_erase(&tty_minors, minor);
}
static int gb_tty_install(struct tty_driver *driver, struct tty_struct *tty)
@@ -837,7 +833,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
minor = alloc_minor(gb_tty);
if (minor < 0) {
- if (minor == -ENOSPC) {
+ if (minor == -EBUSY) {
dev_err(&gbphy_dev->dev,
"no more free minor numbers\n");
retval = -ENODEV;
@@ -982,7 +978,7 @@ static void gb_tty_exit(void)
{
tty_unregister_driver(gb_tty_driver);
put_tty_driver(gb_tty_driver);
- idr_destroy(&tty_minors);
+ xa_destroy(&tty_minors);
}
static const struct gbphy_device_id gb_uart_id_table[] = {
--
2.32.0