[PATCH] media: atomisp: fix memleak in ia_css_stream_create
When aspect_ratio_crop_init() fails, curr_stream needs
to be freed just like what we've done in the following
error paths. However, current code is returning directly
and ends up leaking memory.
Signed-off-by: Dinghao Liu
---
drivers/staging/media/atomisp/pci/sh_css.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/atomisp/pci/sh_css.c
b/drivers/staging/media/atomisp/pci/sh_css.c
index 54434c2dbaf9..8473e1437074 100644
--- a/drivers/staging/media/atomisp/pci/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/sh_css.c
@@ -9521,7 +9521,7 @@ ia_css_stream_create(const struct ia_css_stream_config
*stream_config,
if (err)
{
IA_CSS_LEAVE_ERR(err);
- return err;
+ goto ERR;
}
#endif
for (i = 0; i < num_pipes; i++)
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC 1/3] Initialize devlink health dump framework for the dlge driver
On 2020-08-21 11:08 +0800, Coiby Xu wrote: [...] > > > diff --git a/drivers/staging/qlge/qlge_health.h > > > b/drivers/staging/qlge/qlge_health.h > > > new file mode 100644 > > > index ..07d3bafab845 > > > --- /dev/null > > > +++ b/drivers/staging/qlge/qlge_health.h > > > @@ -0,0 +1,2 @@ > > > +#include > > > +int qlge_health_create_reporters(struct qlge_devlink *priv); > > > > I would suggest to put this in qlge.h instead of creating a new file. > > Although there are only two lines for now, is it possible qlge will add > more devlink code? If that's the case, a file to single out these code I would say that if there's more content in the future, it can move to a separate file in the future. If you feel strongly about putting this in its own file right away, then make sure to add the usual #ifndef QLGE_HEALTH_H #define QLGE_HEALTH_H ... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed
On Fri, 2020-08-21 at 10:42 +0800, Nicolas Boichat wrote:
> On Fri, Aug 21, 2020 at 10:36 AM Joe Perches wrote:
> > On Thu, 2020-08-20 at 21:57 -0400, Steven Rostedt wrote:
> > > On Fri, 21 Aug 2020 09:39:19 +0800
> > > Nicolas Boichat wrote:
> > []
> > > > Some other approaches/ideas:
> > > > 1. Filter all lkml messages that contain trace_printk. Already found
> > > > 1 instance, and I can easily reply to those with a semi-canned answer,
> > > > if I remember to check that filter regularly (not sustainable in the
> > > > long run...).
> > >
> > > Added Joe Perches to the thread.
> > >
> > > We can update checkpatch.pl to complain about a trace_printk() that it
> > > finds in the added code.
> >
> > Why?
> >
> > I don't see much value in a trace_printk checkpatch warning.
> > tracing is still dependent on CONFIG_TRACING otherwise
> > trace_printk is an if (0)
> >
> > ELI5 please.
>
> This is my "new" canned answer to this:
>
> Please do not use trace_printk in production code [1,2], it is only
> meant for debug use. Consider using trace events, or dev_dbg.
> [1] https://elixir.bootlin.com/linux/v5.8/source/kernel/trace/trace.c#L3158
> [2] https://elixir.bootlin.com/linux/v5.8/source/include/linux/kernel.h#L766
>
> I also had arguments in patch 2/3 notes:
>
> There's at least 3 reasons that I can come up with:
> 1. trace_printk introduces some overhead. [some users, e.g.
> Android/Chrome OS, want CONFIG_TRACING but _not_ that extra overhead]
> 2. If the kernel keeps adding always-enabled trace_printk, it will be
> much harder for developers to make use of trace_printk for debugging.
> 3. People may assume that trace_printk is for debugging only, and may
> accidentally output sensitive data (theoretical at this stage).
Perhaps make trace_printk dependent on #define DEBUG?
Something like:
---
include/linux/kernel.h | 7 +++
1 file changed, 7 insertions(+)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 500def620d8f..6ca8f958df73 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -717,6 +717,7 @@ do {
\
* let gcc optimize the rest.
*/
+#ifdef DEBUG
#define trace_printk(fmt, ...) \
do { \
char ___STR[] = __stringify((__VA_ARGS__)); \
@@ -725,6 +726,12 @@ do {
\
else\
trace_puts(fmt);\
} while (0)
+#else
+#define trace_printk(fmt, ...) \
+do { \
+ __trace_printk_check_format(fmt, ##args); \
+} while (0)
+#endif
#define do_trace_printk(fmt, args...) \
do { \
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC 3/3] staging: qlge: clean up code that dump info to dmesg
On Sun, Aug 16, 2020 at 11:57:17AM +0900, Benjamin Poirier wrote:
On 2020-08-15 00:06 +0800, Coiby Xu wrote:
The related code are not necessary because,
- Device status and general registers can be obtained by ethtool.
- Coredump can be done via devlink health reporter.
- Structure related to the hardware (struct ql_adapter) can be obtained
by crash or drgn.
I would suggest to add the drgn script from the cover letter to
Documentation/networking/device_drivers/qlogic/
Thank you for this suggestion! I planned to send a pull request to
https://github.com/osandov/drgn. This is a better idea.
I would also suggest to submit a separate patch now which fixes the
build breakage reported in <20200629053004.GA6165@f3> while you work on
removing that code.
I'll send a single patch to fix that issue before preparing for v1
of this work.
Signed-off-by: Coiby Xu
---
drivers/staging/qlge/qlge.h | 82
drivers/staging/qlge/qlge_dbg.c | 672
drivers/staging/qlge/qlge_ethtool.c | 1 -
drivers/staging/qlge/qlge_main.c| 6 -
4 files changed, 761 deletions(-)
[...]
diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
index 058889687907..368394123d16 100644
--- a/drivers/staging/qlge/qlge_dbg.c
+++ b/drivers/staging/qlge/qlge_dbg.c
@@ -1326,675 +1326,3 @@ void ql_mpi_core_to_log(struct work_struct *work)
sizeof(*qdev->mpi_coredump), false);
}
-#ifdef QL_REG_DUMP
-static void ql_dump_intr_states(struct ql_adapter *qdev)
-{
[...]
- }
-}
-#endif
This leaves a stray newline at the end of the file and also does not
apply over latest staging.
I will fix it in v1. Thank you for reviewing this patch!
--
Best regards,
Coiby
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC 1/3] Initialize devlink health dump framework for the dlge driver
On Sun, Aug 16, 2020 at 11:56:40AM +0900, Benjamin Poirier wrote:
On 2020-08-15 00:05 +0800, Coiby Xu wrote:
Initialize devlink health dump framework for the dlge driver so the
coredump could be done via devlink.
Signed-off-by: Coiby Xu
---
drivers/staging/qlge/Makefile | 2 +-
drivers/staging/qlge/qlge.h| 9 +++
drivers/staging/qlge/qlge_health.c | 43 ++
drivers/staging/qlge/qlge_health.h | 2 ++
drivers/staging/qlge/qlge_main.c | 21 +++
5 files changed, 76 insertions(+), 1 deletion(-)
create mode 100644 drivers/staging/qlge/qlge_health.c
create mode 100644 drivers/staging/qlge/qlge_health.h
diff --git a/drivers/staging/qlge/Makefile b/drivers/staging/qlge/Makefile
index 1dc2568e820c..0a1e4c8dd546 100644
--- a/drivers/staging/qlge/Makefile
+++ b/drivers/staging/qlge/Makefile
@@ -5,4 +5,4 @@
obj-$(CONFIG_QLGE) += qlge.o
-qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o
+qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o qlge_health.o
diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
index fc8c5ca8935d..055ded6dab60 100644
--- a/drivers/staging/qlge/qlge.h
+++ b/drivers/staging/qlge/qlge.h
@@ -2061,6 +2061,14 @@ struct nic_operations {
int (*port_initialize) (struct ql_adapter *);
};
This patch doesn't apply over the latest staging tree. I think your tree
is missing commit d923bb6bf508 ("staging: qlge: qlge.h: Function
definition arguments should have names.")
Thank you for applying the patch to test it! I had incorrect
understanding about the word "RFC" and didn't do a rebase onto
the latest staging tree.
+
+
+struct qlge_devlink {
+struct ql_adapter *qdev;
+struct net_device *ndev;
I don't have experience implementing devlink callbacks but looking at
some other devlink users (mlx4, ionic, ice), all of them use devlink
priv space for their main private structure. That would be struct
ql_adapter in this case. Is there a good reason to stray from that
pattern?
+struct devlink_health_reporter *reporter;
+};
+
/*
* The main Adapter structure definition.
* This structure has all fields relevant to the hardware.
@@ -2078,6 +2086,7 @@ struct ql_adapter {
struct pci_dev *pdev;
struct net_device *ndev;/* Parent NET device */
+ struct qlge_devlink *devlink;
/* Hardware information */
u32 chip_rev_id;
u32 fw_rev_id;
diff --git a/drivers/staging/qlge/qlge_health.c
b/drivers/staging/qlge/qlge_health.c
new file mode 100644
index ..292f6b1827e1
--- /dev/null
+++ b/drivers/staging/qlge/qlge_health.c
@@ -0,0 +1,43 @@
+#include "qlge.h"
+#include "qlge_health.h"
+
+static int
+qlge_reporter_coredump(struct devlink_health_reporter *reporter,
+ struct devlink_fmsg *fmsg, void *priv_ctx,
+ struct netlink_ext_ack *extack)
+{
+ return 0;
+}
+
+static const struct devlink_health_reporter_ops qlge_reporter_ops = {
+ .name = "dummy",
+ .dump = qlge_reporter_coredump,
+};
I think
select NET_DEVLINK
should be added to drivers/staging/qlge/Kconfig
Thank you for reminding me!
+
+int qlge_health_create_reporters(struct qlge_devlink *priv)
+{
+ int err;
+
+ struct devlink_health_reporter *reporter;
+ struct devlink *devlink;
+
+ devlink = priv_to_devlink(priv);
+ reporter =
+ devlink_health_reporter_create(devlink, _reporter_ops,
+ 0,
+ priv);
+ if (IS_ERR(reporter)) {
+ netdev_warn(priv->ndev,
+ "Failed to create reporter, err = %ld\n",
+ PTR_ERR(reporter));
+ return PTR_ERR(reporter);
+ }
+ priv->reporter = reporter;
+
+ if (err)
+ return err;
+
+ return 0;
+}
+
+
Stray newlines
Will fix it in v1.
diff --git a/drivers/staging/qlge/qlge_health.h
b/drivers/staging/qlge/qlge_health.h
new file mode 100644
index ..07d3bafab845
--- /dev/null
+++ b/drivers/staging/qlge/qlge_health.h
@@ -0,0 +1,2 @@
+#include
+int qlge_health_create_reporters(struct qlge_devlink *priv);
I would suggest to put this in qlge.h instead of creating a new file.
Although there are only two lines for now, is it possible qlge will add
more devlink code? If that's the case, a file to single out these code
is necessary as is the same to some other drivers,
$ find drivers -name *health*.h
drivers/net/ethernet/mellanox/mlx5/core/en/health.h
$ find drivers -name *devlink*.h
drivers/net/ethernet/huawei/hinic/hinic_devlink.h
drivers/net/ethernet/mellanox/mlx5/core/devlink.h
drivers/net/ethernet/mellanox/mlx5/core/en/devlink.h
drivers/net/ethernet/intel/ice/ice_devlink.h
drivers/net/ethernet/pensando/ionic/ionic_devlink.h
Re: [PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed
On Thu, 20 Aug 2020 23:04:59 -0400 Steven Rostedt wrote: > On Thu, 20 Aug 2020 19:49:59 -0700 > Joe Perches wrote: > > > Perhaps make trace_printk dependent on #define DEBUG? > > This is basically what Nicolas's patch series does in this very patch! > > And no, I hate it. We are currently discussing ways of not having to > modify the config in order to allow trace_printk() to be used. > > We don't want to burden the developer to take a config, add a bunch of > trace_printks() and find that it's compiled out! > This also breaks another use case. You may be working on a module for a production kernel. It is fine to include trace_printk() in your module, and load it on the production kernel. You will get that banner when you load your module, but that's OK because it is still under development. But something like this change will prevent that from happening. -- Steve ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed
On Thu, 20 Aug 2020 19:49:59 -0700
Joe Perches wrote:
> Perhaps make trace_printk dependent on #define DEBUG?
This is basically what Nicolas's patch series does in this very patch!
And no, I hate it. We are currently discussing ways of not having to
modify the config in order to allow trace_printk() to be used.
We don't want to burden the developer to take a config, add a bunch of
trace_printks() and find that it's compiled out!
Thus, this is a NAK.
-- Steve
>
> Something like:
> ---
> include/linux/kernel.h | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 500def620d8f..6ca8f958df73 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -717,6 +717,7 @@ do {
> \
> * let gcc optimize the rest.
> */
>
> +#ifdef DEBUG
> #define trace_printk(fmt, ...) \
> do { \
> char ___STR[] = __stringify((__VA_ARGS__)); \
> @@ -725,6 +726,12 @@ do {
> \
> else\
> trace_puts(fmt);\
> } while (0)
> +#else
> +#define trace_printk(fmt, ...)
> \
> +do { \
> + __trace_printk_check_format(fmt, ##args); \
> +} while (0)
> +#endif
>
> #define do_trace_printk(fmt, args...)
> \
> do { \
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed
On Fri, 21 Aug 2020 10:39:02 +0800
Nicolas Boichat wrote:
> I'm not sure how that helps? I mean, the use case you have in mind is
> somebody reusing a distro/random config and not being able to use
> trace_printk, right? If that config has CONFIG_DISABLE_TRACE_PRINTK=y,
> then the developer will still need to flip that back.
>
> Note that the option I'm added has default=y (_allow_ trace_printk),
> so I don't think default y or default n really matters?
Ideally, the production system doesn't have it set. It only sets it to
make sure that it's clean before sending out. But then it can add it
back before production. Yeah, it's pretty much cutting hairs between
the two. I don't like either one.
Really, if you are worried about this, just add your patch to your
local tree. I'm not sure this is something that can be fixed upstream.
Another idea is to add something like below, and build with:
make CHECK_TRACE_PRINT=1
This way it is a build command line option that causes the build to
fail if trace_printk() is added.
This way production systems can add this to make sure their kernels are
free of trace_printk() but it doesn't affect the config that is used.
-- Steve
[ Not even compiled tested! ]
diff --git a/Makefile b/Makefile
index 2057c92a6205..5714a738879d 100644
--- a/Makefile
+++ b/Makefile
@@ -91,6 +91,13 @@ else
Q = @
endif
+ifeq ("$(origin CHECK_TRACE_PRINTK)", "command line")
+ KBUILD_NO_TRACE_PRINTK = $(NO_TRACE_PRINTK)
+endif
+ifndef KBUILD_NO_TRACE_PRINTK
+ KBUILD_NO_TRACE_PRINTK = 0
+endif
+
# If the user is running make -s (silent mode), suppress echoing of
# commands
@@ -839,6 +846,10 @@ KBUILD_AFLAGS += -gz=zlib
KBUILD_LDFLAGS += --compress-debug-sections=zlib
endif
+ifeq ($(KBUILD_NO_TRACE_PRINTK),1)
+KBUILD_CFLAGS += -DNO_TRACE_PRINTK
+endif
+
KBUILD_CFLAGS += $(DEBUG_CFLAGS)
export DEBUG_CFLAGS
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 500def620d8f..bee432547d26 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -680,11 +680,14 @@ extern void tracing_stop(void);
static inline __printf(1, 2)
void trace_printk_check_format(const char *fmt, ...)
{
+#ifdef NO_TRACE_PRINTK
+ extern void __no_trace_printk_on_build(void);
+ __no_trace_printk_on_build();
+#endif
}
#define __trace_printk_check_format(fmt, args...) \
do { \
- if (0) \
- trace_printk_check_format(fmt, ##args); \
+ trace_printk_check_format(fmt, ##args); \
} while (0)
/**
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed
On Thu, 20 Aug 2020 19:36:19 -0700 Joe Perches wrote: > On Thu, 2020-08-20 at 21:57 -0400, Steven Rostedt wrote: > > On Fri, 21 Aug 2020 09:39:19 +0800 > > Nicolas Boichat wrote: > [] > > > Some other approaches/ideas: > > > 1. Filter all lkml messages that contain trace_printk. Already found > > > 1 instance, and I can easily reply to those with a semi-canned answer, > > > if I remember to check that filter regularly (not sustainable in the > > > long run...). > > > > Added Joe Perches to the thread. > > > > We can update checkpatch.pl to complain about a trace_printk() that it > > finds in the added code. > > Why? > > I don't see much value in a trace_printk checkpatch warning. > tracing is still dependent on CONFIG_TRACING otherwise > trace_printk is an if (0) > > ELI5 please. > Because no production code should contain trace_printk(). It should be deleted before going to Linus. If you have trace_printk() in your code, you will be greeted by the following banner in your dmesg: ** ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ** ** ** ** trace_printk() being used. Allocating extra memory. ** ** ** ** This means that this is a DEBUG kernel and it is ** ** unsafe for production use. ** ** ** ** If you see this message and you are not debugging** ** the kernel, report this immediately to your vendor! ** ** ** ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ** ** -- Steve ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed
On Fri, Aug 21, 2020 at 10:36 AM Joe Perches wrote: > > On Thu, 2020-08-20 at 21:57 -0400, Steven Rostedt wrote: > > On Fri, 21 Aug 2020 09:39:19 +0800 > > Nicolas Boichat wrote: > [] > > > Some other approaches/ideas: > > > 1. Filter all lkml messages that contain trace_printk. Already found > > > 1 instance, and I can easily reply to those with a semi-canned answer, > > > if I remember to check that filter regularly (not sustainable in the > > > long run...). > > > > Added Joe Perches to the thread. > > > > We can update checkpatch.pl to complain about a trace_printk() that it > > finds in the added code. > > Why? > > I don't see much value in a trace_printk checkpatch warning. > tracing is still dependent on CONFIG_TRACING otherwise > trace_printk is an if (0) > > ELI5 please. This is my "new" canned answer to this: Please do not use trace_printk in production code [1,2], it is only meant for debug use. Consider using trace events, or dev_dbg. [1] https://elixir.bootlin.com/linux/v5.8/source/kernel/trace/trace.c#L3158 [2] https://elixir.bootlin.com/linux/v5.8/source/include/linux/kernel.h#L766 I also had arguments in patch 2/3 notes: There's at least 3 reasons that I can come up with: 1. trace_printk introduces some overhead. [some users, e.g. Android/Chrome OS, want CONFIG_TRACING but _not_ that extra overhead] 2. If the kernel keeps adding always-enabled trace_printk, it will be much harder for developers to make use of trace_printk for debugging. 3. People may assume that trace_printk is for debugging only, and may accidentally output sensitive data (theoretical at this stage). (we'll need to summarize that somehow if we want to add to checkpatch.pl) ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed
On Fri, Aug 21, 2020 at 9:57 AM Steven Rostedt wrote: > > On Fri, 21 Aug 2020 09:39:19 +0800 > Nicolas Boichat wrote: > > > On Fri, Aug 21, 2020 at 8:36 AM Steven Rostedt wrote: > > > > > > On Fri, 21 Aug 2020 08:13:00 +0800 > > > Nicolas Boichat wrote: > > > > > > > On Thu, Aug 20, 2020 at 10:23 PM Steven Rostedt > > > > wrote: > > > > > > > > > > On Thu, 20 Aug 2020 17:14:12 +0800 > > > > > Nicolas Boichat wrote: > > > > > > > > > > > Technically, we could only initialize the trace_printk buffers > > > > > > when the print env is switched, to avoid the build error and > > > > > > unconditional boot-time warning, but I assume this printing > > > > > > framework will eventually get removed when the driver moves out > > > > > > of staging? > > > > > > > > > > Perhaps this should be converting into a trace event. Look at what bpf > > > > > did for their bpf_trace_printk(). > > > > > > > > > > The more I think about it, the less I like this series. > > > > > > > > To make it clear, the primary goal of this series is to get rid of > > > > trace_printk sprinkled in the kernel by making sure some randconfig > > > > builds fail. Since my v2, there already has been one more added (the > > > > one that this patch removes), so I'd like to land 2/3 ASAP to prevent > > > > even more from being added. > > > > > > > > Looking at your reply on 1/3, I think we are aligned on that goal? Is > > > > there some other approach you'd recommend? > > > > > > > > Now, I'm not pretending my fixes are the best possible ones, but I > > > > would much rather have the burden of converting to trace events on the > > > > respective driver maintainers. (btw is there a short > > > > documentation/tutorial that I could link to in these patches, to help > > > > developers understand what is the recommended way now?) > > > > > > > > > > I like the goal, but I guess I never articulated the problem I have > > > with the methodology. > > > > > > trace_printk() is meant to be a debugging tool. Something that people > > > can and do sprinkle all over the kernel to help them find a bug in > > > areas that are called quite often (where printk() is way too slow). > > > > > > The last thing I want them to deal with is adding a trace_printk() with > > > their distro's config (or a config from someone that triggered the bug) > > > only to have the build to fail, because they also need to add a config > > > value. > > > > > > I add to the Cc a few developers I know that use trace_printk() in this > > > fashion. I'd like to hear their view on having to add a config option > > > to make trace_printk work before they test a config that is sent to > > > them. > > > > Gotcha, thanks. I have also used trace_printk in the past, as > > uncommitted changes (and understand the usefulness ,-)). And in Chrome > > OS team here, developers have also raised this concern: how do we make > > the developer flow convenient so that we can add trace_printk to our > > code for debugging, without having to flip back that config option, > > and _yet_ make sure that no trace_printk ever makes it into our > > production kernels. We have creative ways of making that work (portage > > USE flags and stuff). But I'm not sure about other flows, and your > > concern is totally valid... > > > > Some other approaches/ideas: > > 1. Filter all lkml messages that contain trace_printk. Already found > > 1 instance, and I can easily reply to those with a semi-canned answer, > > if I remember to check that filter regularly (not sustainable in the > > long run...). > > Added Joe Perches to the thread. > > We can update checkpatch.pl to complain about a trace_printk() that it > finds in the added code. Oh, that's a good and simple idea. > > > 2. Integration into some kernel test robot? (I will not roll my own > > for this ,-)) It may be a bit difficult as some debug config options > > do enable trace_printk, and that's ok. > > 3. In Chromium OS, I can add a unit test (i.e. something outside of > > the normal kernel build system), but that'll only catch regressions > > downstream (or when we happen to backport patches). > > > > Down the line, #3 catches what I care about the most (Chromium OS > > issues: we had production kernels for a few days/weeks showing that > > splat on boot), but it'd be nice to have something upstream that > > benefits everyone. > > > > What about an opposite config. That is, not have a config to enable it. > But one to disable it. If it is disabled and a trace_printk is found, > it will fail the build. This way your builds will not allow your kernel > to get out the door with one. > > #ifdef CONFIG_DISABLE_TRACE_PRINTK > #define trace_printk__this_function_is_disabled > #endif I'm not sure how that helps? I mean, the use case you have in mind is somebody reusing a distro/random config and not being able to use trace_printk, right? If that config has CONFIG_DISABLE_TRACE_PRINTK=y, then the developer will still need to flip that back. Note that the
Re: [PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed
On Fri, 21 Aug 2020 09:39:19 +0800 Nicolas Boichat wrote: > On Fri, Aug 21, 2020 at 8:36 AM Steven Rostedt wrote: > > > > On Fri, 21 Aug 2020 08:13:00 +0800 > > Nicolas Boichat wrote: > > > > > On Thu, Aug 20, 2020 at 10:23 PM Steven Rostedt > > > wrote: > > > > > > > > On Thu, 20 Aug 2020 17:14:12 +0800 > > > > Nicolas Boichat wrote: > > > > > > > > > Technically, we could only initialize the trace_printk buffers > > > > > when the print env is switched, to avoid the build error and > > > > > unconditional boot-time warning, but I assume this printing > > > > > framework will eventually get removed when the driver moves out > > > > > of staging? > > > > > > > > Perhaps this should be converting into a trace event. Look at what bpf > > > > did for their bpf_trace_printk(). > > > > > > > > The more I think about it, the less I like this series. > > > > > > To make it clear, the primary goal of this series is to get rid of > > > trace_printk sprinkled in the kernel by making sure some randconfig > > > builds fail. Since my v2, there already has been one more added (the > > > one that this patch removes), so I'd like to land 2/3 ASAP to prevent > > > even more from being added. > > > > > > Looking at your reply on 1/3, I think we are aligned on that goal? Is > > > there some other approach you'd recommend? > > > > > > Now, I'm not pretending my fixes are the best possible ones, but I > > > would much rather have the burden of converting to trace events on the > > > respective driver maintainers. (btw is there a short > > > documentation/tutorial that I could link to in these patches, to help > > > developers understand what is the recommended way now?) > > > > > > > I like the goal, but I guess I never articulated the problem I have > > with the methodology. > > > > trace_printk() is meant to be a debugging tool. Something that people > > can and do sprinkle all over the kernel to help them find a bug in > > areas that are called quite often (where printk() is way too slow). > > > > The last thing I want them to deal with is adding a trace_printk() with > > their distro's config (or a config from someone that triggered the bug) > > only to have the build to fail, because they also need to add a config > > value. > > > > I add to the Cc a few developers I know that use trace_printk() in this > > fashion. I'd like to hear their view on having to add a config option > > to make trace_printk work before they test a config that is sent to > > them. > > Gotcha, thanks. I have also used trace_printk in the past, as > uncommitted changes (and understand the usefulness ,-)). And in Chrome > OS team here, developers have also raised this concern: how do we make > the developer flow convenient so that we can add trace_printk to our > code for debugging, without having to flip back that config option, > and _yet_ make sure that no trace_printk ever makes it into our > production kernels. We have creative ways of making that work (portage > USE flags and stuff). But I'm not sure about other flows, and your > concern is totally valid... > > Some other approaches/ideas: > 1. Filter all lkml messages that contain trace_printk. Already found > 1 instance, and I can easily reply to those with a semi-canned answer, > if I remember to check that filter regularly (not sustainable in the > long run...). Added Joe Perches to the thread. We can update checkpatch.pl to complain about a trace_printk() that it finds in the added code. > 2. Integration into some kernel test robot? (I will not roll my own > for this ,-)) It may be a bit difficult as some debug config options > do enable trace_printk, and that's ok. > 3. In Chromium OS, I can add a unit test (i.e. something outside of > the normal kernel build system), but that'll only catch regressions > downstream (or when we happen to backport patches). > > Down the line, #3 catches what I care about the most (Chromium OS > issues: we had production kernels for a few days/weeks showing that > splat on boot), but it'd be nice to have something upstream that > benefits everyone. > What about an opposite config. That is, not have a config to enable it. But one to disable it. If it is disabled and a trace_printk is found, it will fail the build. This way your builds will not allow your kernel to get out the door with one. #ifdef CONFIG_DISABLE_TRACE_PRINTK #define trace_printk__this_function_is_disabled #endif ? -- Steve ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed
On Fri, Aug 21, 2020 at 8:36 AM Steven Rostedt wrote: > > On Fri, 21 Aug 2020 08:13:00 +0800 > Nicolas Boichat wrote: > > > On Thu, Aug 20, 2020 at 10:23 PM Steven Rostedt wrote: > > > > > > On Thu, 20 Aug 2020 17:14:12 +0800 > > > Nicolas Boichat wrote: > > > > > > > Technically, we could only initialize the trace_printk buffers > > > > when the print env is switched, to avoid the build error and > > > > unconditional boot-time warning, but I assume this printing > > > > framework will eventually get removed when the driver moves out > > > > of staging? > > > > > > Perhaps this should be converting into a trace event. Look at what bpf > > > did for their bpf_trace_printk(). > > > > > > The more I think about it, the less I like this series. > > > > To make it clear, the primary goal of this series is to get rid of > > trace_printk sprinkled in the kernel by making sure some randconfig > > builds fail. Since my v2, there already has been one more added (the > > one that this patch removes), so I'd like to land 2/3 ASAP to prevent > > even more from being added. > > > > Looking at your reply on 1/3, I think we are aligned on that goal? Is > > there some other approach you'd recommend? > > > > Now, I'm not pretending my fixes are the best possible ones, but I > > would much rather have the burden of converting to trace events on the > > respective driver maintainers. (btw is there a short > > documentation/tutorial that I could link to in these patches, to help > > developers understand what is the recommended way now?) > > > > I like the goal, but I guess I never articulated the problem I have > with the methodology. > > trace_printk() is meant to be a debugging tool. Something that people > can and do sprinkle all over the kernel to help them find a bug in > areas that are called quite often (where printk() is way too slow). > > The last thing I want them to deal with is adding a trace_printk() with > their distro's config (or a config from someone that triggered the bug) > only to have the build to fail, because they also need to add a config > value. > > I add to the Cc a few developers I know that use trace_printk() in this > fashion. I'd like to hear their view on having to add a config option > to make trace_printk work before they test a config that is sent to > them. Gotcha, thanks. I have also used trace_printk in the past, as uncommitted changes (and understand the usefulness ,-)). And in Chrome OS team here, developers have also raised this concern: how do we make the developer flow convenient so that we can add trace_printk to our code for debugging, without having to flip back that config option, and _yet_ make sure that no trace_printk ever makes it into our production kernels. We have creative ways of making that work (portage USE flags and stuff). But I'm not sure about other flows, and your concern is totally valid... Some other approaches/ideas: 1. Filter all lkml messages that contain trace_printk. Already found 1 instance, and I can easily reply to those with a semi-canned answer, if I remember to check that filter regularly (not sustainable in the long run...). 2. Integration into some kernel test robot? (I will not roll my own for this ,-)) It may be a bit difficult as some debug config options do enable trace_printk, and that's ok. 3. In Chromium OS, I can add a unit test (i.e. something outside of the normal kernel build system), but that'll only catch regressions downstream (or when we happen to backport patches). Down the line, #3 catches what I care about the most (Chromium OS issues: we had production kernels for a few days/weeks showing that splat on boot), but it'd be nice to have something upstream that benefits everyone. Thanks, > > -- Steve ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed
On Fri, 21 Aug 2020 08:13:00 +0800 Nicolas Boichat wrote: > On Thu, Aug 20, 2020 at 10:23 PM Steven Rostedt wrote: > > > > On Thu, 20 Aug 2020 17:14:12 +0800 > > Nicolas Boichat wrote: > > > > > Technically, we could only initialize the trace_printk buffers > > > when the print env is switched, to avoid the build error and > > > unconditional boot-time warning, but I assume this printing > > > framework will eventually get removed when the driver moves out > > > of staging? > > > > Perhaps this should be converting into a trace event. Look at what bpf > > did for their bpf_trace_printk(). > > > > The more I think about it, the less I like this series. > > To make it clear, the primary goal of this series is to get rid of > trace_printk sprinkled in the kernel by making sure some randconfig > builds fail. Since my v2, there already has been one more added (the > one that this patch removes), so I'd like to land 2/3 ASAP to prevent > even more from being added. > > Looking at your reply on 1/3, I think we are aligned on that goal? Is > there some other approach you'd recommend? > > Now, I'm not pretending my fixes are the best possible ones, but I > would much rather have the burden of converting to trace events on the > respective driver maintainers. (btw is there a short > documentation/tutorial that I could link to in these patches, to help > developers understand what is the recommended way now?) > I like the goal, but I guess I never articulated the problem I have with the methodology. trace_printk() is meant to be a debugging tool. Something that people can and do sprinkle all over the kernel to help them find a bug in areas that are called quite often (where printk() is way too slow). The last thing I want them to deal with is adding a trace_printk() with their distro's config (or a config from someone that triggered the bug) only to have the build to fail, because they also need to add a config value. I add to the Cc a few developers I know that use trace_printk() in this fashion. I'd like to hear their view on having to add a config option to make trace_printk work before they test a config that is sent to them. -- Steve ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed
On Thu, Aug 20, 2020 at 10:23 PM Steven Rostedt wrote: > > On Thu, 20 Aug 2020 17:14:12 +0800 > Nicolas Boichat wrote: > > > Technically, we could only initialize the trace_printk buffers > > when the print env is switched, to avoid the build error and > > unconditional boot-time warning, but I assume this printing > > framework will eventually get removed when the driver moves out > > of staging? > > Perhaps this should be converting into a trace event. Look at what bpf > did for their bpf_trace_printk(). > > The more I think about it, the less I like this series. To make it clear, the primary goal of this series is to get rid of trace_printk sprinkled in the kernel by making sure some randconfig builds fail. Since my v2, there already has been one more added (the one that this patch removes), so I'd like to land 2/3 ASAP to prevent even more from being added. Looking at your reply on 1/3, I think we are aligned on that goal? Is there some other approach you'd recommend? Now, I'm not pretending my fixes are the best possible ones, but I would much rather have the burden of converting to trace events on the respective driver maintainers. (btw is there a short documentation/tutorial that I could link to in these patches, to help developers understand what is the recommended way now?) Thanks, > > -- Steve ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RESEND v10 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset
Hi Anday,
On Tue, Aug 18, 2020 at 4:14 AM Andy Shevchenko
wrote:
>
> On Mon, Aug 17, 2020 at 05:53:09PM -0400, Jim Quinlan wrote:
> > The new field 'dma_range_map' in struct device is used to facilitate the
> > use of single or multiple offsets between mapping regions of cpu addrs and
> > dma addrs. It subsumes the role of "dev->dma_pfn_offset" which was only
> > capable of holding a single uniform offset and had no region bounds
> > checking.
> >
> > The function of_dma_get_range() has been modified so that it takes a single
> > argument -- the device node -- and returns a map, NULL, or an error code.
> > The map is an array that holds the information regarding the DMA regions.
> > Each range entry contains the address offset, the cpu_start address, the
> > dma_start address, and the size of the region.
> >
> > of_dma_configure() is the typical manner to set range offsets but there are
> > a number of ad hoc assignments to "dev->dma_pfn_offset" in the kernel
> > driver code. These cases now invoke the function
> > dma_attach_offset_range(dev, cpu_addr, dma_addr, size).
>
> ...
>
> > + if (dev) {
> > + phys_addr_t paddr = PFN_PHYS(pfn);
> > +
>
> > + pfn -= (dma_offset_from_phys_addr(dev, paddr) >> PAGE_SHIFT);
>
> PFN_DOWN() ?
Yep.
>
> > + }
>
> ...
>
> > + pfn += (dma_offset_from_dma_addr(dev, addr) >> PAGE_SHIFT);
>
> Ditto.
Yep.
>
>
> ...
>
> > +static inline u64 dma_offset_from_dma_addr(struct device *dev, dma_addr_t
> > dma_addr)
> > +{
> > + const struct bus_dma_region *m = dev->dma_range_map;
> > +
> > + if (!m)
> > + return 0;
> > + for (; m->size; m++)
> > + if (dma_addr >= m->dma_start && dma_addr - m->dma_start <
> > m->size)
> > + return m->offset;
> > + return 0;
> > +}
> > +
> > +static inline u64 dma_offset_from_phys_addr(struct device *dev,
> > phys_addr_t paddr)
> > +{
> > + const struct bus_dma_region *m = dev->dma_range_map;
> > +
> > + if (!m)
> > + return 0;
> > + for (; m->size; m++)
> > + if (paddr >= m->cpu_start && paddr - m->cpu_start < m->size)
> > + return m->offset;
> > + return 0;
> > +}
>
> Perhaps for these the form with one return 0 is easier to read
>
> if (m) {
> for (; m->size; m++)
> if (paddr >= m->cpu_start && paddr - m->cpu_start <
> m->size)
> return m->offset;
> }
> return 0;
>
> ?
I see what you are saying but I don't think there is enough difference
between the two to justify changing it.
>
> ...
>
> > + if (mem->use_dev_dma_pfn_offset) {
> > + u64 base_addr = (u64)mem->pfn_base << PAGE_SHIFT;
>
> PFN_PHYS() ?
Yep.
>
> > +
> > + return base_addr - dma_offset_from_phys_addr(dev, base_addr);
> > + }
>
> ...
>
> > + * It returns -ENOMEM if out of memory, 0 otherwise.
>
> This doesn't describe cases dev->dma_range_map != NULL and offset == 0.
Okay, I'll fix this.
>
> > +int dma_set_offset_range(struct device *dev, phys_addr_t cpu_start,
> > + dma_addr_t dma_start, u64 size)
> > +{
> > + struct bus_dma_region *map;
> > + u64 offset = (u64)cpu_start - (u64)dma_start;
> > +
> > + if (!offset)
> > + return 0;
> > +
> > + if (dev->dma_range_map) {
> > + dev_err(dev, "attempt to add DMA range to existing map\n");
> > + return -EINVAL;
> > + }
> > +
> > + map = kcalloc(2, sizeof(*map), GFP_KERNEL);
> > + if (!map)
> > + return -ENOMEM;
> > + map[0].cpu_start = cpu_start;
> > + map[0].dma_start = dma_start;
> > + map[0].offset = offset;
> > + map[0].size = size;
> > + dev->dma_range_map = map;
> > +
> > + return 0;
> > +}
>
> ...
>
> > +void *dma_copy_dma_range_map(const struct bus_dma_region *map)
> > +{
> > + int num_ranges;
> > + struct bus_dma_region *new_map;
> > + const struct bus_dma_region *r = map;
> > +
> > + for (num_ranges = 0; r->size; num_ranges++)
> > + r++;
>
> > + new_map = kcalloc(num_ranges + 1, sizeof(*map), GFP_KERNEL);
> > + if (new_map)
> > + memcpy(new_map, map, sizeof(*map) * num_ranges);
>
> Looks like krealloc() on the first glance...
It's not. We are making a distinct copy of the original, not resizing it.
>
> > +
> > + return new_map;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks again,
Jim
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 04/12] staging: wfx: fix spaces around binary operators
From: Jérôme Pouiller
A binary operator should be followed by exactly one space.
Signed-off-by: Jérôme Pouiller
---
drivers/staging/wfx/key.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/wfx/key.c b/drivers/staging/wfx/key.c
index 5ee2ffc5f935..6165df59ecf9 100644
--- a/drivers/staging/wfx/key.c
+++ b/drivers/staging/wfx/key.c
@@ -171,7 +171,7 @@ static int wfx_add_key(struct wfx_vif *wvif, struct
ieee80211_sta *sta,
k.int_id = wvif->id;
k.entry_index = idx;
if (key->cipher == WLAN_CIPHER_SUITE_WEP40 ||
- key->cipher == WLAN_CIPHER_SUITE_WEP104) {
+ key->cipher == WLAN_CIPHER_SUITE_WEP104) {
if (pairwise)
k.type = fill_wep_pair(_pairwise_key, key,
sta->addr);
@@ -191,13 +191,13 @@ static int wfx_add_key(struct wfx_vif *wvif, struct
ieee80211_sta *sta,
else
k.type = fill_ccmp_group(_group_key, key,
);
- } else if (key->cipher == WLAN_CIPHER_SUITE_SMS4) {
+ } else if (key->cipher == WLAN_CIPHER_SUITE_SMS4) {
if (pairwise)
k.type = fill_sms4_pair(_pairwise_key, key,
sta->addr);
else
k.type = fill_sms4_group(_group_key, key);
- } else if (key->cipher == WLAN_CIPHER_SUITE_AES_CMAC) {
+ } else if (key->cipher == WLAN_CIPHER_SUITE_AES_CMAC) {
k.type = fill_aes_cmac_group(_group_key, key,
);
} else {
--
2.28.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 05/12] staging: wfx: fix support for cipher AES_CMAC (multicast PMF)
From: Jérôme Pouiller
When MFP is enabled, the multicast management frames are not protected,
in fact. Instead, but they should include an IE containing the MMIC of
the frames (i.e. a cryptographic signature).
Until now, the driver didn't correctly detect this kind of frames (they
are not marked protected but they are associated to a key) and didn't
ask to the device to encrypt them.
In add, the device is not able to generate the IE itself. Mac80211 has
to generate the IE and let the device compute the MMIC.
Signed-off-by: Jérôme Pouiller
---
drivers/staging/wfx/data_tx.c | 5 +++--
drivers/staging/wfx/key.c | 4 ++--
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
index 41f9afd41e14..d16b516ad7cf 100644
--- a/drivers/staging/wfx/data_tx.c
+++ b/drivers/staging/wfx/data_tx.c
@@ -325,6 +325,8 @@ static int wfx_tx_get_icv_len(struct ieee80211_key_conf
*hw_key)
if (!hw_key)
return 0;
+ if (hw_key->cipher == WLAN_CIPHER_SUITE_AES_CMAC)
+ return 0;
mic_space = (hw_key->cipher == WLAN_CIPHER_SUITE_TKIP) ? 8 : 0;
return hw_key->icv_len + mic_space;
}
@@ -350,8 +352,7 @@ static int wfx_tx_inner(struct wfx_vif *wvif, struct
ieee80211_sta *sta,
memset(tx_info->rate_driver_data, 0, sizeof(struct wfx_tx_priv));
// Fill tx_priv
tx_priv = (struct wfx_tx_priv *)tx_info->rate_driver_data;
- if (ieee80211_has_protected(hdr->frame_control))
- tx_priv->hw_key = hw_key;
+ tx_priv->hw_key = hw_key;
// Fill hif_msg
WARN(skb_headroom(skb) < wmsg_len, "not enough space in skb");
diff --git a/drivers/staging/wfx/key.c b/drivers/staging/wfx/key.c
index 6165df59ecf9..728e5f8d3b7c 100644
--- a/drivers/staging/wfx/key.c
+++ b/drivers/staging/wfx/key.c
@@ -198,8 +198,8 @@ static int wfx_add_key(struct wfx_vif *wvif, struct
ieee80211_sta *sta,
else
k.type = fill_sms4_group(_group_key, key);
} else if (key->cipher == WLAN_CIPHER_SUITE_AES_CMAC) {
- k.type = fill_aes_cmac_group(_group_key, key,
-);
+ k.type = fill_aes_cmac_group(_group_key, key, );
+ key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIE;
} else {
dev_warn(wdev->dev, "unsupported key type %d\n", key->cipher);
wfx_free_key(wdev, idx);
--
2.28.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 07/12] staging: wfx: fix frame reordering
From: Jérôme Pouiller
When mac80211 debug is enabled, the trace below appears:
[60744.340037] wlan0: Rx A-MPDU request on aa:bb:cc:97:60:24 tid 0 result
-524
This imply that ___ieee80211_start_rx_ba_session will prematurely exit
and frame reordering won't be enabled.
Fixes: e5da5fbd77411 ("staging: wfx: fix CCMP/TKIP replay protection")
Signed-off-by: Jérôme Pouiller
---
drivers/staging/wfx/sta.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index 20db4bbdd901..b18a0b61b7c0 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -674,15 +674,16 @@ int wfx_ampdu_action(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
struct ieee80211_ampdu_params *params)
{
- /* Aggregation is implemented fully in firmware,
-* including block ack negotiation. Do not allow
-* mac80211 stack to do anything: it interferes with
-* the firmware.
-*/
-
- /* Note that we still need this function stubbed. */
-
- return -ENOTSUPP;
+ // Aggregation is implemented fully in firmware
+ switch (params->action) {
+ case IEEE80211_AMPDU_RX_START:
+ case IEEE80211_AMPDU_RX_STOP:
+ // Just acknowledge it to enable frame re-ordering
+ return 0;
+ default:
+ // Leave the firmware doing its business for tx aggregation
+ return -ENOTSUPP;
+ }
}
int wfx_add_chanctx(struct ieee80211_hw *hw,
--
2.28.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 08/12] staging: wfx: fix potential use before init
From: Jérôme Pouiller
The trace below can appear:
[83613.832200] INFO: trying to register non-static key.
[83613.837248] the code is fine but needs lockdep annotation.
[83613.842808] turning off the locking correctness validator.
[83613.848375] CPU: 3 PID: 141 Comm: kworker/3:2H Tainted: G O
5.6.13-silabs15 #2
[83613.857019] Hardware name: BCM2835
[83613.860605] Workqueue: events_highpri bh_work [wfx]
[83613.865552] Backtrace:
[83613.868041] [] (dump_backtrace) from []
(show_stack+0x20/0x24)
[83613.881463] [] (show_stack) from []
(dump_stack+0xe8/0x114)
[83613.82] [] (dump_stack) from []
(register_lock_class+0x748/0x768)
[83613.905035] [] (register_lock_class) from []
(__lock_acquire+0x88/0x13dc)
[83613.924192] [] (__lock_acquire) from []
(lock_acquire+0xe8/0x274)
[83613.942644] [] (lock_acquire) from []
(_raw_spin_lock_irqsave+0x58/0x6c)
[83613.961714] [] (_raw_spin_lock_irqsave) from []
(skb_dequeue+0x24/0x78)
[83613.974967] [] (skb_dequeue) from []
(wfx_tx_queues_get+0x96c/0x1294 [wfx])
[83613.989728] [] (wfx_tx_queues_get [wfx]) from []
(bh_work+0x454/0x26d8 [wfx])
[83614.009337] [] (bh_work [wfx]) from []
(process_one_work+0x23c/0x7ec)
[83614.028141] [] (process_one_work) from []
(worker_thread+0x4c/0x55c)
[83614.046861] [] (worker_thread) from []
(kthread+0x138/0x168)
[83614.064876] [] (kthread) from []
(ret_from_fork+0x14/0x20)
[83614.072200] Exception stack(0xecad3fb0 to 0xecad3ff8)
[83614.077323] 3fa0:
[83614.085620] 3fc0:
[83614.093914] 3fe0: 0013
Indeed, the code of wfx_add_interface() shows that the interface is
enabled to early. So, the spinlock associated with some skb_queue may
not yet initialized when wfx_tx_queues_get() is called.
Signed-off-by: Jérôme Pouiller
---
drivers/staging/wfx/sta.c | 30 +++---
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index b18a0b61b7c0..9b760fb574f8 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -753,17 +753,6 @@ int wfx_add_interface(struct ieee80211_hw *hw, struct
ieee80211_vif *vif)
return -EOPNOTSUPP;
}
- for (i = 0; i < ARRAY_SIZE(wdev->vif); i++) {
- if (!wdev->vif[i]) {
- wdev->vif[i] = vif;
- wvif->id = i;
- break;
- }
- }
- if (i == ARRAY_SIZE(wdev->vif)) {
- mutex_unlock(>conf_mutex);
- return -EOPNOTSUPP;
- }
// FIXME: prefer use of container_of() to get vif
wvif->vif = vif;
wvif->wdev = wdev;
@@ -780,12 +769,22 @@ int wfx_add_interface(struct ieee80211_hw *hw, struct
ieee80211_vif *vif)
init_completion(>scan_complete);
INIT_WORK(>scan_work, wfx_hw_scan_work);
- mutex_unlock(>conf_mutex);
-
- hif_set_macaddr(wvif, vif->addr);
-
wfx_tx_queues_init(wvif);
wfx_tx_policy_init(wvif);
+
+ for (i = 0; i < ARRAY_SIZE(wdev->vif); i++) {
+ if (!wdev->vif[i]) {
+ wdev->vif[i] = vif;
+ wvif->id = i;
+ break;
+ }
+ }
+ WARN(i == ARRAY_SIZE(wdev->vif), "try to instantiate more vif than
supported");
+
+ hif_set_macaddr(wvif, vif->addr);
+
+ mutex_unlock(>conf_mutex);
+
wvif = NULL;
while ((wvif = wvif_iterate(wdev, wvif)) != NULL) {
// Combo mode does not support Block Acks. We can re-enable them
@@ -817,6 +816,7 @@ void wfx_remove_interface(struct ieee80211_hw *hw, struct
ieee80211_vif *vif)
wvif->vif = NULL;
mutex_unlock(>conf_mutex);
+
wvif = NULL;
while ((wvif = wvif_iterate(wdev, wvif)) != NULL) {
// Combo mode does not support Block Acks. We can re-enable them
--
2.28.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 09/12] staging: wfx: scan while AP is supported
From: Jérôme Pouiller The device is able to scan while running an Access Point. Just declare it. Signed-off-by: Jérôme Pouiller --- drivers/staging/wfx/main.c | 1 + drivers/staging/wfx/scan.c | 4 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c index 4263f912760b..5a3018e14445 100644 --- a/drivers/staging/wfx/main.c +++ b/drivers/staging/wfx/main.c @@ -282,6 +282,7 @@ struct wfx_dev *wfx_init_common(struct device *dev, NL80211_PROBE_RESP_OFFLOAD_SUPPORT_WPS2 | NL80211_PROBE_RESP_OFFLOAD_SUPPORT_P2P | NL80211_PROBE_RESP_OFFLOAD_SUPPORT_80211U; + hw->wiphy->features |= NL80211_FEATURE_AP_SCAN; hw->wiphy->flags |= WIPHY_FLAG_AP_PROBE_RESP_OFFLOAD; hw->wiphy->flags |= WIPHY_FLAG_AP_UAPSD; hw->wiphy->flags &= ~WIPHY_FLAG_PS_ON_BY_DEFAULT; diff --git a/drivers/staging/wfx/scan.c b/drivers/staging/wfx/scan.c index e9de19784865..02d4e653d594 100644 --- a/drivers/staging/wfx/scan.c +++ b/drivers/staging/wfx/scan.c @@ -113,10 +113,6 @@ int wfx_hw_scan(struct ieee80211_hw *hw, struct ieee80211_vif *vif, struct wfx_vif *wvif = (struct wfx_vif *)vif->drv_priv; WARN_ON(hw_req->req.n_channels > HIF_API_MAX_NB_CHANNELS); - - if (vif->type == NL80211_IFTYPE_AP) - return -EOPNOTSUPP; - wvif->scan_req = hw_req; schedule_work(>scan_work); return 0; -- 2.28.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 06/12] staging: wfx: drop useless field from struct wfx_tx_priv
From: Jérôme Pouiller
The device need to receive a skb with necessary space for the ICV. So,
the driver adds this space before to send the frame.
Currently, once the frame is sent, the driver restore the original
content of the skb. However, this step is useless. Mac80211 don't do it
when software encryption is enabled.
Once we have removed this step, it appears that it is no more necessary
to keep hw_key in tx_priv. Then, it is possible to simplify a bunch of
code in the Tx path.
Signed-off-by: Jérôme Pouiller
---
drivers/staging/wfx/data_tx.c | 16
drivers/staging/wfx/data_tx.h | 3 +--
2 files changed, 5 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
index d16b516ad7cf..485907b0faa2 100644
--- a/drivers/staging/wfx/data_tx.c
+++ b/drivers/staging/wfx/data_tx.c
@@ -336,7 +336,6 @@ static int wfx_tx_inner(struct wfx_vif *wvif, struct
ieee80211_sta *sta,
{
struct hif_msg *hif_msg;
struct hif_req_tx *req;
- struct wfx_tx_priv *tx_priv;
struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
struct ieee80211_key_conf *hw_key = tx_info->control.hw_key;
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
@@ -350,14 +349,11 @@ static int wfx_tx_inner(struct wfx_vif *wvif, struct
ieee80211_sta *sta,
// From now tx_info->control is unusable
memset(tx_info->rate_driver_data, 0, sizeof(struct wfx_tx_priv));
- // Fill tx_priv
- tx_priv = (struct wfx_tx_priv *)tx_info->rate_driver_data;
- tx_priv->hw_key = hw_key;
// Fill hif_msg
WARN(skb_headroom(skb) < wmsg_len, "not enough space in skb");
WARN(offset & 1, "attempt to transmit an unaligned frame");
- skb_put(skb, wfx_tx_get_icv_len(tx_priv->hw_key));
+ skb_put(skb, wfx_tx_get_icv_len(hw_key));
skb_push(skb, wmsg_len);
memset(skb->data, 0, wmsg_len);
hif_msg = (struct hif_msg *)skb->data;
@@ -489,7 +485,6 @@ static void wfx_tx_fill_rates(struct wfx_dev *wdev,
void wfx_tx_confirm_cb(struct wfx_dev *wdev, const struct hif_cnf_tx *arg)
{
struct ieee80211_tx_info *tx_info;
- const struct wfx_tx_priv *tx_priv;
struct wfx_vif *wvif;
struct sk_buff *skb;
@@ -499,18 +494,15 @@ void wfx_tx_confirm_cb(struct wfx_dev *wdev, const struct
hif_cnf_tx *arg)
arg->packet_id);
return;
}
+ tx_info = IEEE80211_SKB_CB(skb);
wvif = wdev_to_wvif(wdev, ((struct hif_msg *)skb->data)->interface);
WARN_ON(!wvif);
if (!wvif)
return;
- tx_info = IEEE80211_SKB_CB(skb);
- tx_priv = wfx_skb_tx_priv(skb);
+
+ // Note that wfx_pending_get_pkt_us_delay() get data from tx_info
_trace_tx_stats(arg, skb, wfx_pending_get_pkt_us_delay(wdev, skb));
-
- // You can touch to tx_priv, but don't touch to tx_info->status.
wfx_tx_fill_rates(wdev, tx_info, arg);
- skb_trim(skb, skb->len - wfx_tx_get_icv_len(tx_priv->hw_key));
-
// From now, you can touch to tx_info->status, but do not touch to
// tx_priv anymore
// FIXME: use ieee80211_tx_info_clear_status()
diff --git a/drivers/staging/wfx/data_tx.h b/drivers/staging/wfx/data_tx.h
index cff7b9ff99a9..87e1b9b62dbb 100644
--- a/drivers/staging/wfx/data_tx.h
+++ b/drivers/staging/wfx/data_tx.h
@@ -35,8 +35,7 @@ struct tx_policy_cache {
struct wfx_tx_priv {
ktime_t xmit_timestamp;
- struct ieee80211_key_conf *hw_key;
-} __packed;
+};
void wfx_tx_policy_init(struct wfx_vif *wvif);
void wfx_tx_policy_upload_work(struct work_struct *work);
--
2.28.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 01/12] staging: wfx: fix BA when device is AP and MFP is enabled
From: Jérôme Pouiller
The protection of the management frames is mainly done by mac80211.
However, frames for the management of the BlockAck sessions are directly
sent by the device. These frames have to be protected if MFP is in use.
So the driver has to pass the MFP configuration to the device.
Until now, the BlockAck management frames were completely unprotected
whatever the status of the MFP negotiation. So, some devices dropped
these frames.
The device has two knobs to control the MFP. One global and one per
station. Normally, the driver should always enable global MFP. Then it
should enable MFP on every station with which MFP was successfully
negotiated. Unfortunately, the older firmwares only provide the
global control.
So, this patch enable global MFP as it is exposed in the beacon. Then it
marks every station with which the MFP is effective.
Thus, the support for the old firmwares is not so bad. It may only
encounter some difficulties to negotiate BA sessions when the local
device (the AP) is MFP capable (ieee80211w=1) but the station is not.
The only solution for this case is to upgrade the firmware.
Signed-off-by: Jérôme Pouiller
---
drivers/staging/wfx/sta.c | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index ad63332f690c..9c1c8223a49f 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -434,7 +434,7 @@ int wfx_sta_add(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
wvif->link_id_map |= BIT(sta_priv->link_id);
WARN_ON(!sta_priv->link_id);
WARN_ON(sta_priv->link_id >= HIF_LINK_ID_MAX);
- hif_map_link(wvif, sta->addr, 0, sta_priv->link_id);
+ hif_map_link(wvif, sta->addr, sta->mfp ? 2 : 0, sta_priv->link_id);
return 0;
}
@@ -474,6 +474,25 @@ static int wfx_upload_ap_templates(struct wfx_vif *wvif)
return 0;
}
+static void wfx_set_mfp_ap(struct wfx_vif *wvif)
+{
+ struct sk_buff *skb = ieee80211_beacon_get(wvif->wdev->hw, wvif->vif);
+ const int ieoffset = offsetof(struct ieee80211_mgmt, u.beacon.variable);
+ const u16 *ptr = (u16 *)cfg80211_find_ie(WLAN_EID_RSN,
+skb->data + ieoffset,
+skb->len - ieoffset);
+ const int pairwise_cipher_suite_count_offset = 8 / sizeof(u16);
+ const int pairwise_cipher_suite_size = 4 / sizeof(u16);
+ const int akm_suite_size = 4 / sizeof(u16);
+
+ if (ptr) {
+ ptr += pairwise_cipher_suite_count_offset;
+ ptr += 1 + pairwise_cipher_suite_size * *ptr;
+ ptr += 1 + akm_suite_size * *ptr;
+ hif_set_mfp(wvif, *ptr & BIT(7), *ptr & BIT(6));
+ }
+}
+
int wfx_start_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif)
{
struct wfx_vif *wvif = (struct wfx_vif *)vif->drv_priv;
@@ -488,6 +507,7 @@ int wfx_start_ap(struct ieee80211_hw *hw, struct
ieee80211_vif *vif)
ret = hif_start(wvif, >bss_conf, wvif->channel);
if (ret > 0)
return -EIO;
+ wfx_set_mfp_ap(wvif);
return ret;
}
--
2.28.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 02/12] staging: wfx: improve usage of hif_map_link()
From: Jérôme Pouiller
Until now, hif_map_link() get as argument the raw value for
map_link_flags when map_link_flags is defined as a bitfield. It was
error prone.
Now hif_map_link() takes explicit value for every flags of the
struct map_link_flags.
Signed-off-by: Jérôme Pouiller
---
drivers/staging/wfx/hif_tx.c | 5 +++--
drivers/staging/wfx/hif_tx.h | 3 ++-
drivers/staging/wfx/sta.c| 4 ++--
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index 3b5f4dcc469c..6b89e55f03f4 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c
@@ -499,7 +499,7 @@ int hif_beacon_transmit(struct wfx_vif *wvif, bool enable)
return ret;
}
-int hif_map_link(struct wfx_vif *wvif, u8 *mac_addr, int flags, int sta_id)
+int hif_map_link(struct wfx_vif *wvif, bool unmap, u8 *mac_addr, int sta_id,
bool mfp)
{
int ret;
struct hif_msg *hif;
@@ -509,7 +509,8 @@ int hif_map_link(struct wfx_vif *wvif, u8 *mac_addr, int
flags, int sta_id)
return -ENOMEM;
if (mac_addr)
ether_addr_copy(body->mac_addr, mac_addr);
- body->map_link_flags = *(struct hif_map_link_flags *)
+ body->map_link_flags.mfpc = mfp ? 1 : 0;
+ body->map_link_flags.map_direction = unmap ? 1 : 0;
body->peer_sta_id = sta_id;
wfx_fill_header(hif, wvif->id, HIF_REQ_ID_MAP_LINK, sizeof(*body));
ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
diff --git a/drivers/staging/wfx/hif_tx.h b/drivers/staging/wfx/hif_tx.h
index e1da28aef706..b371b3777a31 100644
--- a/drivers/staging/wfx/hif_tx.h
+++ b/drivers/staging/wfx/hif_tx.h
@@ -55,7 +55,8 @@ int hif_set_edca_queue_params(struct wfx_vif *wvif, u16 queue,
int hif_start(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
const struct ieee80211_channel *channel);
int hif_beacon_transmit(struct wfx_vif *wvif, bool enable);
-int hif_map_link(struct wfx_vif *wvif, u8 *mac_addr, int flags, int sta_id);
+int hif_map_link(struct wfx_vif *wvif,
+bool unmap, u8 *mac_addr, int sta_id, bool mfp);
int hif_update_ie_beacon(struct wfx_vif *wvif, const u8 *ies, size_t ies_len);
int hif_sl_set_mac_key(struct wfx_dev *wdev,
const u8 *slk_key, int destination);
diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index 9c1c8223a49f..d2e9cf651c81 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -434,7 +434,7 @@ int wfx_sta_add(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
wvif->link_id_map |= BIT(sta_priv->link_id);
WARN_ON(!sta_priv->link_id);
WARN_ON(sta_priv->link_id >= HIF_LINK_ID_MAX);
- hif_map_link(wvif, sta->addr, sta->mfp ? 2 : 0, sta_priv->link_id);
+ hif_map_link(wvif, false, sta->addr, sta_priv->link_id, sta->mfp);
return 0;
}
@@ -449,7 +449,7 @@ int wfx_sta_remove(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
if (!sta_priv->link_id)
return 0;
// FIXME add a mutex?
- hif_map_link(wvif, sta->addr, 1, sta_priv->link_id);
+ hif_map_link(wvif, true, sta->addr, sta_priv->link_id, false);
wvif->link_id_map &= ~BIT(sta_priv->link_id);
return 0;
}
--
2.28.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 03/12] staging: wfx: fix BA when MFP is disabled but BSS is MFP capable
From: Jérôme Pouiller
The protection of the management frames is mainly done by mac80211.
However, frames for the management of the BlockAck sessions are directly
sent by the device. These frames have to be protected if MFP is in use.
So the driver has to pass the MFP configuration to the device.
Until now, the driver directly read the RSN IE of the BSS. However, it
didn't work when the BSS was MFP capable (ieee80211w=1) and the local
device has disabled MFP (ieee80211w=0).
This patch read the MFP information directly from the struct
ieee80211_sta. This information take into account the MFP negotiated
during the association. In addition, the code is far simpler.
Signed-off-by: Jérôme Pouiller
---
drivers/staging/wfx/sta.c | 34 +++---
1 file changed, 3 insertions(+), 31 deletions(-)
diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
index d2e9cf651c81..20db4bbdd901 100644
--- a/drivers/staging/wfx/sta.c
+++ b/drivers/staging/wfx/sta.c
@@ -323,36 +323,6 @@ void wfx_set_default_unicast_key(struct ieee80211_hw *hw,
hif_wep_default_key_id(wvif, idx);
}
-static void wfx_set_mfp(struct wfx_vif *wvif,
- struct cfg80211_bss *bss)
-{
- const int pairwise_cipher_suite_count_offset = 8 / sizeof(u16);
- const int pairwise_cipher_suite_size = 4 / sizeof(u16);
- const int akm_suite_size = 4 / sizeof(u16);
- const u16 *ptr = NULL;
- bool mfpc = false;
- bool mfpr = false;
-
- /* 802.11w protected mgmt frames */
-
- /* retrieve MFPC and MFPR flags from beacon or PBRSP */
-
- rcu_read_lock();
- if (bss)
- ptr = (const u16 *)ieee80211_bss_get_ie(bss, WLAN_EID_RSN);
-
- if (ptr) {
- ptr += pairwise_cipher_suite_count_offset;
- ptr += 1 + pairwise_cipher_suite_size * *ptr;
- ptr += 1 + akm_suite_size * *ptr;
- mfpr = *ptr & BIT(6);
- mfpc = *ptr & BIT(7);
- }
- rcu_read_unlock();
-
- hif_set_mfp(wvif, mfpc, mfpr);
-}
-
void wfx_reset(struct wfx_vif *wvif)
{
struct wfx_dev *wdev = wvif->wdev;
@@ -400,7 +370,6 @@ static void wfx_do_join(struct wfx_vif *wvif)
}
rcu_read_unlock();
- wfx_set_mfp(wvif, bss);
cfg80211_put_bss(wvif->wdev->hw->wiphy, bss);
wvif->join_in_progress = true;
@@ -427,6 +396,9 @@ int wfx_sta_add(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
sta_priv->vif_id = wvif->id;
+ if (vif->type == NL80211_IFTYPE_STATION)
+ hif_set_mfp(wvif, sta->mfp, sta->mfp);
+
// In station mode, the firmware interprets new link-id as a TDLS peer.
if (vif->type == NL80211_IFTYPE_STATION && !sta->tdls)
return 0;
--
2.28.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 12/12] staging: wfx: add workaround for 'timeout while wake up chip'
From: Jérôme Pouiller
The host and the device can be connected with a called Wake-Up GPIO.
When the host fall down this GPIO, it allows the device to enter in deep
sleep and no communication with the device is no more possible (the
device wakes up automatically on DTIM and fetch data if necessary).
So, before to communicate with the device, the driver have to raise the
Wake-up GPIO and then wait for an IRQ from the device.
Unfortunately, old firmwares have a race in sleep/wake-up process and
the device may never wake up. In this case, the IRQ is not sent and
driver complains with "timeout while wake up chip". Then, the driver
tries anyway to access the bus and an other error is raised by the bus.
Fortunately, when the bug occurs, it is possible to fall down the IRQ
and the device will eventually finish the sleep process. Then the driver
can wake it up normally.
The patch implements that workaround and add a retry limit in case
something goes very wrong.
Signed-off-by: Jérôme Pouiller
---
drivers/staging/wfx/bh.c | 23 +++
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
index 07304a80c29b..f07bcee50e3f 100644
--- a/drivers/staging/wfx/bh.c
+++ b/drivers/staging/wfx/bh.c
@@ -18,25 +18,40 @@
static void device_wakeup(struct wfx_dev *wdev)
{
+ int max_retry = 3;
+
if (!wdev->pdata.gpio_wakeup)
return;
if (gpiod_get_value_cansleep(wdev->pdata.gpio_wakeup))
return;
- gpiod_set_value_cansleep(wdev->pdata.gpio_wakeup, 1);
if (wfx_api_older_than(wdev, 1, 4)) {
+ gpiod_set_value_cansleep(wdev->pdata.gpio_wakeup, 1);
if (!completion_done(>hif.ctrl_ready))
usleep_range(2000, 2500);
- } else {
+ return;
+ }
+ for (;;) {
+ gpiod_set_value_cansleep(wdev->pdata.gpio_wakeup, 1);
// completion.h does not provide any function to wait
// completion without consume it (a kind of
// wait_for_completion_done_timeout()). So we have to emulate
// it.
if (wait_for_completion_timeout(>hif.ctrl_ready,
- msecs_to_jiffies(2)))
+ msecs_to_jiffies(2))) {
complete(>hif.ctrl_ready);
- else
+ return;
+ } else if (max_retry-- > 0) {
+ // Older firmwares have a race in sleep/wake-up process.
+ // Redo the process is sufficient to unfreeze the
+ // chip.
dev_err(wdev->dev, "timeout while wake up chip\n");
+ gpiod_set_value_cansleep(wdev->pdata.gpio_wakeup, 0);
+ usleep_range(2000, 2500);
+ } else {
+ dev_err(wdev->dev, "max wake-up retries reached\n");
+ return;
+ }
}
}
--
2.28.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 11/12] staging: wfx: remove useless extra jiffy
From: Jérôme Pouiller The initial developer has feared msecs_to_jiffies() could round down the result. However, the documentation of msecs_to_jiffies() says that the result is rounded upward. So the increment of the result of msecs_to_jiffies() is not necessary. Signed-off-by: Jérôme Pouiller --- drivers/staging/wfx/bh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c index 53ae0b5abcdd..07304a80c29b 100644 --- a/drivers/staging/wfx/bh.c +++ b/drivers/staging/wfx/bh.c @@ -33,7 +33,7 @@ static void device_wakeup(struct wfx_dev *wdev) // wait_for_completion_done_timeout()). So we have to emulate // it. if (wait_for_completion_timeout(>hif.ctrl_ready, - msecs_to_jiffies(2) + 1)) + msecs_to_jiffies(2))) complete(>hif.ctrl_ready); else dev_err(wdev->dev, "timeout while wake up chip\n"); -- 2.28.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 10/12] staging: wfx: enable powersave on probe
From: Jérôme Pouiller In the old days, ieee80211 powersave has some impact on the Rx speed. These problems are solved for a long time now. There is no more reason to not enabling it. Signed-off-by: Jérôme Pouiller --- drivers/staging/wfx/main.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c index 5a3018e14445..5e2b82499004 100644 --- a/drivers/staging/wfx/main.c +++ b/drivers/staging/wfx/main.c @@ -285,7 +285,6 @@ struct wfx_dev *wfx_init_common(struct device *dev, hw->wiphy->features |= NL80211_FEATURE_AP_SCAN; hw->wiphy->flags |= WIPHY_FLAG_AP_PROBE_RESP_OFFLOAD; hw->wiphy->flags |= WIPHY_FLAG_AP_UAPSD; - hw->wiphy->flags &= ~WIPHY_FLAG_PS_ON_BY_DEFAULT; hw->wiphy->max_ap_assoc_sta = HIF_LINK_ID_MAX; hw->wiphy->max_scan_ssids = 2; hw->wiphy->max_scan_ie_len = IEEE80211_MAX_DATA_LEN; -- 2.28.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] ANDROID: binder: print warnings when detecting oneway spamming.
The most common cause of the binder transaction buffer filling up is a
client rapidly firing oneway transactions into a process, before it has
a chance to handle them. Yet the root cause of this is often hard to
debug, because either the system or the app will stop, and by that time
binder debug information we dump in bugreports is no longer relevant.
This change warns as soon as a process dips below 80% of its oneway
space (less than 100kB available in the configuration), when any one
process is responsible for either more than 50 transactions, or more
than 50% of the oneway space.
Signed-off-by: Martijn Coenen
---
v2: fixed call-site in binder_alloc_selftest
drivers/android/binder.c| 2 +-
drivers/android/binder_alloc.c | 49 +++--
drivers/android/binder_alloc.h | 5 ++-
drivers/android/binder_alloc_selftest.c | 2 +-
4 files changed, 52 insertions(+), 6 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f936530a19b0..946332bc871a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3136,7 +3136,7 @@ static void binder_transaction(struct binder_proc *proc,
t->buffer = binder_alloc_new_buf(_proc->alloc, tr->data_size,
tr->offsets_size, extra_buffers_size,
- !reply && (t->flags & TF_ONE_WAY));
+ !reply && (t->flags & TF_ONE_WAY), current->tgid);
if (IS_ERR(t->buffer)) {
/*
* -ESRCH indicates VMA cleared. The target is dying.
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 69609696a843..76e8e633dbd4 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -338,12 +338,48 @@ static inline struct vm_area_struct *binder_alloc_get_vma(
return vma;
}
+static void debug_low_async_space_locked(struct binder_alloc *alloc, int pid)
+{
+ /*
+* Find the amount and size of buffers allocated by the current caller;
+* The idea is that once we cross the threshold, whoever is responsible
+* for the low async space is likely to try to send another async txn,
+* and at some point we'll catch them in the act. This is more efficient
+* than keeping a map per pid.
+*/
+ struct rb_node *n = alloc->free_buffers.rb_node;
+ struct binder_buffer *buffer;
+ size_t buffer_size;
+ size_t total_alloc_size = 0;
+ size_t num_buffers = 0;
+
+ for (n = rb_first(>allocated_buffers); n != NULL;
+n = rb_next(n)) {
+ buffer = rb_entry(n, struct binder_buffer, rb_node);
+ if (buffer->pid != pid)
+ continue;
+ if (!buffer->async_transaction)
+ continue;
+ buffer_size = binder_alloc_buffer_size(alloc, buffer);
+ total_alloc_size += buffer_size;
+ num_buffers++;
+ }
+
+ // Warn if this pid has more than 50% of async space, or more than 50
txns
+ if (num_buffers > 50 || total_alloc_size > alloc->buffer_size / 4) {
+ binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
+"%d: pid %d spamming oneway? %zd buffers allocated
for a total size of %zd\n",
+ alloc->pid, pid, num_buffers, total_alloc_size);
+ }
+}
+
static struct binder_buffer *binder_alloc_new_buf_locked(
struct binder_alloc *alloc,
size_t data_size,
size_t offsets_size,
size_t extra_buffers_size,
- int is_async)
+ int is_async,
+ int pid)
{
struct rb_node *n = alloc->free_buffers.rb_node;
struct binder_buffer *buffer;
@@ -486,11 +522,16 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
buffer->offsets_size = offsets_size;
buffer->async_transaction = is_async;
buffer->extra_buffers_size = extra_buffers_size;
+ buffer->pid = pid;
if (is_async) {
alloc->free_async_space -= size + sizeof(struct binder_buffer);
binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC_ASYNC,
"%d: binder_alloc_buf size %zd async free %zd\n",
alloc->pid, size, alloc->free_async_space);
+ if (alloc->free_async_space < alloc->buffer_size / 10) {
+ // Start detecting spammers once we reach 80% of async
space used
+ debug_low_async_space_locked(alloc, pid);
+ }
}
return buffer;
@@ -508,6 +549,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
* @offsets_size: user specified buffer offset
* @extra_buffers_size: size of extra space for meta-data (eg, security
Re: [PATCH] binder: print warnings when detecting oneway spamming.
On Thu, Aug 20, 2020 at 12:41 PM kernel test robot wrote:
>
> Hi Martijn,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on staging/staging-testing]
> [also build test ERROR on v5.9-rc1 next-20200820]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url:
> https://github.com/0day-ci/linux/commits/Martijn-Coenen/binder-print-warnings-when-detecting-oneway-spamming/20200820-155358
> base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> bc752d2f345bf55d71b3422a6a24890ea03168dc
> config: s390-randconfig-r002-20200818 (attached as .config)
> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project
> 4deda57106f7c9b982a49cb907c33e3966c8de7f)
> reproduce (this is a W=1 build):
> wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # install s390 cross compiling tool for clang build
> # apt-get install binutils-s390x-linux-gnu
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot
>
> All errors (new ones prefixed by >>):
>
> >> drivers/android/binder_alloc_selftest.c:122:61: error: too few arguments
> >> to function call, expected 6, have 5
>buffers[i] = binder_alloc_new_buf(alloc, sizes[i], 0, 0,
> 0);
missed this call-site, will send v2.
Martijn
> ^
>drivers/android/binder_alloc.h:118:30: note: 'binder_alloc_new_buf'
> declared here
>extern struct binder_buffer *binder_alloc_new_buf(struct binder_alloc
> *alloc,
> ^
>1 error generated.
>
> #
> https://github.com/0day-ci/linux/commit/9d0b269f4468d6793f6fd76a410fdde39dbf6ac2
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review
> Martijn-Coenen/binder-print-warnings-when-detecting-oneway-spamming/20200820-155358
> git checkout 9d0b269f4468d6793f6fd76a410fdde39dbf6ac2
> vim +122 drivers/android/binder_alloc_selftest.c
>
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 114
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 115 static void
> binder_selftest_alloc_buf(struct binder_alloc *alloc,
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 116
> struct binder_buffer *buffers[],
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 117
> size_t *sizes, int *seq)
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 118 {
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 119 int i;
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 120
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 121 for (i = 0; i < BUFFER_NUM;
> i++) {
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 @122 buffers[i] =
> binder_alloc_new_buf(alloc, sizes[i], 0, 0, 0);
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 123 if
> (IS_ERR(buffers[i]) ||
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 124
> !check_buffer_pages_allocated(alloc, buffers[i],
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 125
> sizes[i])) {
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 126
> pr_err_size_seq(sizes, seq);
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 127
> binder_selftest_failures++;
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 128 }
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 129 }
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 130 }
> 4175e2b46fd4b9 Sherry Yang 2017-08-23 131
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro.
Quick feedback below.
Sam
On Thu, Aug 20, 2020 at 05:13:22PM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 20 Aug 2020 16:48:08 +0200
> Sam Ravnborg escreveu:
>
> > Hi Mauro.
> >
> > On Thu, Aug 20, 2020 at 04:06:49PM +0200, Mauro Carvalho Chehab wrote:
> > > Em Wed, 19 Aug 2020 19:35:58 +0200
> > > Sam Ravnborg escreveu:
> > >
> > > I'm already handling the other comments from your review (I'll send a
> > > more complete comment about them after finishing),
> > If you get back only on things you do not understand or do not agree on
> > that would be fine. The rest should be visible in the changelog on the
> > updated patch - no need to do extra work here.
> >
> > > but I have a doubt what you meant about this:
> > >
> > > > +static int kirin_drm_bind(struct device *dev)
> > > > > +{
> > > > > + struct drm_driver *driver = _drm_driver;
> > > > > + struct drm_device *drm_dev;
> > > > > + struct kirin_drm_private *priv;
> > > > > + int ret;
> > > > > +
> > > > > + drm_dev = drm_dev_alloc(driver, dev);
> > > > > + if (!drm_dev)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + ret = kirin_drm_kms_init(drm_dev);
> > > > > + if (ret)
> > > > > + goto err_drm_dev_unref;
> > > > > +
> > > > > + ret = drm_dev_register(drm_dev, 0);
> > > > There is better ways to do this. See drm_drv.c for the code example.
> > >
> > > Not sure if I understood your comment here. The drm_drv.c example also
> > > calls
> > > drm_dev_register().
> >
> > This is indeed not obvious from my comments but what I wnated to say is
> > that the driver should embed drm_device in some struct,
> > maybe in "struct kirin_drm_private".
> >
> > This should also be part of the referenced example.
> >
> > I hope this clarifies it.
>
> Yeah. I was already doing those changes ;-)
>
> Something like the enclosed patch, right?
>
> Btw, I'm not sure if the error handling part is ok, as I didn't check
> what the devm stuff does at the subsystem.
>
> -
>
> On a separate question, I was unable to use the helper macros,
> as it sounds that there's no macro with this:
>
> .dumb_create= drm_gem_cma_dumb_create_internal,
>
> The existing DRM_GEM_CMA_VMAP_DRIVER_OPS uses, instead
> drm_gem_cma_dumb_create(). I'm not sure if this driver can use
> such function instead.
>From the documentation of drm_gem_cma_dumb_create_internal:
* It should not be used directly
* as their _driver.dumb_create callback.
I would expect drm_gem_cma_dumb_create() to be the right choice.
So you can go ahead with DRM_GEM_CMA_VMAP_DRIVER_OPS
(I hope I am right, not the are i know much about)
> staging: hikey9xx/gpu: use drm_managed interface
>
> Use a more modern design for the driver binding logic by
> using drm_managed and getting rid of drm->dev_private.
>
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> index c7736f4d74b7..600c89605cc0 100644
> --- a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> +++ b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
> @@ -29,12 +29,13 @@
> #include
> #include
> #include
> +#include
>
> #include "kirin9xx_drm_drv.h"
>
> static int kirin_drm_kms_cleanup(struct drm_device *dev)
> {
> - struct kirin_drm_private *priv = dev->dev_private;
> + struct kirin_drm_private *priv = to_drm_private(dev);
> static struct kirin_dc_ops const *dc_ops;
>
> if (priv->fbdev)
> @@ -45,15 +46,13 @@ static int kirin_drm_kms_cleanup(struct drm_device *dev)
> drm_kms_helper_poll_fini(dev);
> dc_ops->cleanup(dev);
> drm_mode_config_cleanup(dev);
This should also be gone when you are using
drmm_mode_config_init()
> - devm_kfree(dev->dev, priv);
> - dev->dev_private = NULL;
>
> return 0;
> }
>
> static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
> {
> - struct kirin_drm_private *priv = dev->dev_private;
> + struct kirin_drm_private *priv = to_drm_private(dev);
>
> dsi_set_output_client(dev);
>
> @@ -69,18 +68,20 @@ static const struct drm_mode_config_funcs
> kirin_drm_mode_config_funcs = {
>
> static int kirin_drm_kms_init(struct drm_device *dev)
> {
> - struct kirin_drm_private *priv = dev->dev_private;
> + struct kirin_drm_private *priv = to_drm_private(dev);
It is assigned a few lines later.
> static struct kirin_dc_ops const *dc_ops;
> int ret;
>
> - priv = devm_kzalloc(dev->dev, sizeof(*priv), GFP_KERNEL);
> + priv = drmm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> return -ENOMEM;
OK, I am confused here.
This code allocates a struct kirin_drm_private.
But the calling function does the same.
What am I missing here? Coffee?
>
> dev->dev_private = priv;
> dev_set_drvdata(dev->dev, dev);
>
> -
[RESEND PATCH] drivers: most: add character device interface driver
This patch adds the character device (cdev) driver source file
most_cdev.c and modifies the Makefiles and Kconfigs accordingly.
Signed-off-by: Christian Gromm
---
drivers/most/Kconfig | 9 +
drivers/most/Makefile | 1 +
drivers/most/most_cdev.c | 543 +
drivers/staging/most/Kconfig | 2 -
drivers/staging/most/Makefile | 1 -
drivers/staging/most/cdev/Kconfig | 13 -
drivers/staging/most/cdev/Makefile | 4 -
drivers/staging/most/cdev/cdev.c | 543 -
8 files changed, 553 insertions(+), 563 deletions(-)
create mode 100644 drivers/most/most_cdev.c
delete mode 100644 drivers/staging/most/cdev/Kconfig
delete mode 100644 drivers/staging/most/cdev/Makefile
delete mode 100644 drivers/staging/most/cdev/cdev.c
diff --git a/drivers/most/Kconfig b/drivers/most/Kconfig
index 60fc082..ebfe84e 100644
--- a/drivers/most/Kconfig
+++ b/drivers/most/Kconfig
@@ -23,4 +23,13 @@ config MOST_USB_HDM
To compile this driver as a module, choose M here: the
module will be called most_usb.
+
+config MOST_CDEV
+ tristate "Cdev"
+
+ help
+ Say Y here if you want to commumicate via character devices.
+
+ To compile this driver as a module, choose M here: the
+ module will be called most_cdev.
endif
diff --git a/drivers/most/Makefile b/drivers/most/Makefile
index 6a3cb90..8b53ca4 100644
--- a/drivers/most/Makefile
+++ b/drivers/most/Makefile
@@ -4,3 +4,4 @@ most_core-y := core.o \
configfs.o
obj-$(CONFIG_MOST_USB_HDM) += most_usb.o
+obj-$(CONFIG_MOST_CDEV) += most_cdev.o
diff --git a/drivers/most/most_cdev.c b/drivers/most/most_cdev.c
new file mode 100644
index 000..0448807
--- /dev/null
+++ b/drivers/most/most_cdev.c
@@ -0,0 +1,543 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * cdev.c - Character device component for Mostcore
+ *
+ * Copyright (C) 2013-2015 Microchip Technology Germany II GmbH & Co. KG
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define CHRDEV_REGION_SIZE 50
+
+static struct cdev_component {
+ dev_t devno;
+ struct ida minor_id;
+ unsigned int major;
+ struct class *class;
+ struct most_component cc;
+} comp;
+
+struct comp_channel {
+ wait_queue_head_t wq;
+ spinlock_t unlink; /* synchronization lock to unlink channels */
+ struct cdev cdev;
+ struct device *dev;
+ struct mutex io_mutex;
+ struct most_interface *iface;
+ struct most_channel_config *cfg;
+ unsigned int channel_id;
+ dev_t devno;
+ size_t mbo_offs;
+ DECLARE_KFIFO_PTR(fifo, typeof(struct mbo *));
+ int access_ref;
+ struct list_head list;
+};
+
+#define to_channel(d) container_of(d, struct comp_channel, cdev)
+static struct list_head channel_list;
+static spinlock_t ch_list_lock;
+
+static inline bool ch_has_mbo(struct comp_channel *c)
+{
+ return channel_has_mbo(c->iface, c->channel_id, ) > 0;
+}
+
+static inline struct mbo *ch_get_mbo(struct comp_channel *c, struct mbo **mbo)
+{
+ if (!kfifo_peek(>fifo, mbo)) {
+ *mbo = most_get_mbo(c->iface, c->channel_id, );
+ if (*mbo)
+ kfifo_in(>fifo, mbo, 1);
+ }
+ return *mbo;
+}
+
+static struct comp_channel *get_channel(struct most_interface *iface, int id)
+{
+ struct comp_channel *c, *tmp;
+ unsigned long flags;
+
+ spin_lock_irqsave(_list_lock, flags);
+ list_for_each_entry_safe(c, tmp, _list, list) {
+ if ((c->iface == iface) && (c->channel_id == id)) {
+ spin_unlock_irqrestore(_list_lock, flags);
+ return c;
+ }
+ }
+ spin_unlock_irqrestore(_list_lock, flags);
+ return NULL;
+}
+
+static void stop_channel(struct comp_channel *c)
+{
+ struct mbo *mbo;
+
+ while (kfifo_out((struct kfifo *)>fifo, , 1))
+ most_put_mbo(mbo);
+ most_stop_channel(c->iface, c->channel_id, );
+}
+
+static void destroy_cdev(struct comp_channel *c)
+{
+ unsigned long flags;
+
+ device_destroy(comp.class, c->devno);
+ cdev_del(>cdev);
+ spin_lock_irqsave(_list_lock, flags);
+ list_del(>list);
+ spin_unlock_irqrestore(_list_lock, flags);
+}
+
+static void destroy_channel(struct comp_channel *c)
+{
+ ida_simple_remove(_id, MINOR(c->devno));
+ kfifo_free(>fifo);
+ kfree(c);
+}
+
+/**
+ * comp_open - implements the syscall to open the device
+ * @inode: inode pointer
+ * @filp: file pointer
+ *
+ * This stores the channel pointer in the private data field of
+ * the file structure and activates the channel within the core.
+ */
+static int comp_open(struct inode *inode, struct file *filp)
+{
+ struct comp_channel *c;
+ int ret;
+
+ c
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Thu, 20 Aug 2020 16:48:08 +0200
Sam Ravnborg escreveu:
> Hi Mauro.
>
> On Thu, Aug 20, 2020 at 04:06:49PM +0200, Mauro Carvalho Chehab wrote:
> > Em Wed, 19 Aug 2020 19:35:58 +0200
> > Sam Ravnborg escreveu:
> >
> > I'm already handling the other comments from your review (I'll send a
> > more complete comment about them after finishing),
> If you get back only on things you do not understand or do not agree on
> that would be fine. The rest should be visible in the changelog on the
> updated patch - no need to do extra work here.
>
> > but I have a doubt what you meant about this:
> >
> > > +static int kirin_drm_bind(struct device *dev)
> > > > +{
> > > > + struct drm_driver *driver = _drm_driver;
> > > > + struct drm_device *drm_dev;
> > > > + struct kirin_drm_private *priv;
> > > > + int ret;
> > > > +
> > > > + drm_dev = drm_dev_alloc(driver, dev);
> > > > + if (!drm_dev)
> > > > + return -ENOMEM;
> > > > +
> > > > + ret = kirin_drm_kms_init(drm_dev);
> > > > + if (ret)
> > > > + goto err_drm_dev_unref;
> > > > +
> > > > + ret = drm_dev_register(drm_dev, 0);
> > > There is better ways to do this. See drm_drv.c for the code example.
> >
> > Not sure if I understood your comment here. The drm_drv.c example also
> > calls
> > drm_dev_register().
>
> This is indeed not obvious from my comments but what I wnated to say is
> that the driver should embed drm_device in some struct,
> maybe in "struct kirin_drm_private".
>
> This should also be part of the referenced example.
>
> I hope this clarifies it.
Yeah. I was already doing those changes ;-)
Something like the enclosed patch, right?
Btw, I'm not sure if the error handling part is ok, as I didn't check
what the devm stuff does at the subsystem.
-
On a separate question, I was unable to use the helper macros,
as it sounds that there's no macro with this:
.dumb_create= drm_gem_cma_dumb_create_internal,
The existing DRM_GEM_CMA_VMAP_DRIVER_OPS uses, instead
drm_gem_cma_dumb_create(). I'm not sure if this driver can use
such function instead.
Thanks,
Mauro
staging: hikey9xx/gpu: use drm_managed interface
Use a more modern design for the driver binding logic by
using drm_managed and getting rid of drm->dev_private.
Signed-off-by: Mauro Carvalho Chehab
diff --git a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
index c7736f4d74b7..600c89605cc0 100644
--- a/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
+++ b/drivers/staging/hikey9xx/gpu/kirin9xx_drm_drv.c
@@ -29,12 +29,13 @@
#include
#include
#include
+#include
#include "kirin9xx_drm_drv.h"
static int kirin_drm_kms_cleanup(struct drm_device *dev)
{
- struct kirin_drm_private *priv = dev->dev_private;
+ struct kirin_drm_private *priv = to_drm_private(dev);
static struct kirin_dc_ops const *dc_ops;
if (priv->fbdev)
@@ -45,15 +46,13 @@ static int kirin_drm_kms_cleanup(struct drm_device *dev)
drm_kms_helper_poll_fini(dev);
dc_ops->cleanup(dev);
drm_mode_config_cleanup(dev);
- devm_kfree(dev->dev, priv);
- dev->dev_private = NULL;
return 0;
}
static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
{
- struct kirin_drm_private *priv = dev->dev_private;
+ struct kirin_drm_private *priv = to_drm_private(dev);
dsi_set_output_client(dev);
@@ -69,18 +68,20 @@ static const struct drm_mode_config_funcs
kirin_drm_mode_config_funcs = {
static int kirin_drm_kms_init(struct drm_device *dev)
{
- struct kirin_drm_private *priv = dev->dev_private;
+ struct kirin_drm_private *priv = to_drm_private(dev);
static struct kirin_dc_ops const *dc_ops;
int ret;
- priv = devm_kzalloc(dev->dev, sizeof(*priv), GFP_KERNEL);
+ priv = drmm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
dev->dev_private = priv;
dev_set_drvdata(dev->dev, dev);
- drm_mode_config_init(dev);
+ ret = drmm_mode_config_init(dev);
+ if (ret)
+ return ret;
dev->mode_config.min_width = 0;
dev->mode_config.min_height = 0;
@@ -94,20 +95,20 @@ static int kirin_drm_kms_init(struct drm_device *dev)
dc_ops = of_device_get_match_data(dev->dev);
ret = dc_ops->init(dev);
if (ret)
- goto err_mode_config_cleanup;
+ return ret;
/* bind and init sub drivers */
ret = component_bind_all(dev->dev, dev);
if (ret) {
DRM_ERROR("failed to bind all component.\n");
- goto err_dc_cleanup;
+ return ret;
}
/* vblank init */
ret = drm_vblank_init(dev, dev->mode_config.num_crtc);
if (ret) {
DRM_ERROR("failed to initialize vblank.\n");
-
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro.
On Thu, Aug 20, 2020 at 04:06:49PM +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 19 Aug 2020 19:35:58 +0200
> Sam Ravnborg escreveu:
>
> I'm already handling the other comments from your review (I'll send a
> more complete comment about them after finishing),
If you get back only on things you do not understand or do not agree on
that would be fine. The rest should be visible in the changelog on the
updated patch - no need to do extra work here.
> but I have a doubt what you meant about this:
>
> > +static int kirin_drm_bind(struct device *dev)
> > > +{
> > > + struct drm_driver *driver = _drm_driver;
> > > + struct drm_device *drm_dev;
> > > + struct kirin_drm_private *priv;
> > > + int ret;
> > > +
> > > + drm_dev = drm_dev_alloc(driver, dev);
> > > + if (!drm_dev)
> > > + return -ENOMEM;
> > > +
> > > + ret = kirin_drm_kms_init(drm_dev);
> > > + if (ret)
> > > + goto err_drm_dev_unref;
> > > +
> > > + ret = drm_dev_register(drm_dev, 0);
> > There is better ways to do this. See drm_drv.c for the code example.
>
> Not sure if I understood your comment here. The drm_drv.c example also calls
> drm_dev_register().
This is indeed not obvious from my comments but what I wnated to say is
that the driver should embed drm_device in some struct,
maybe in "struct kirin_drm_private".
This should also be part of the referenced example.
I hope this clarifies it.
Sam
>
> The only difference is that it calls it inside driver_probe(), while
> on this driver, it uses:
>
> static const struct component_master_ops kirin_drm_ops = {
> .bind = kirin_drm_bind,
> .unbind = kirin_drm_unbind,
> };
>
> static int kirin_drm_platform_probe(struct platform_device *pdev)
> {
> ...
> drm_of_component_match_add(dev, , compare_of, remote);
> ...
> return component_master_add_with_match(dev, _drm_ops, match);
> }
>
> Are you meaning that I should get rid of this component API and call
> kirin_drm_bind() from kirin_drm_platform_probe()?
>
> Thanks,
> Mauro
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed
On Thu, 20 Aug 2020 17:14:12 +0800 Nicolas Boichat wrote: > Technically, we could only initialize the trace_printk buffers > when the print env is switched, to avoid the build error and > unconditional boot-time warning, but I assume this printing > framework will eventually get removed when the driver moves out > of staging? Perhaps this should be converting into a trace event. Look at what bpf did for their bpf_trace_printk(). The more I think about it, the less I like this series. -- Steve ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Wed, 19 Aug 2020 19:35:58 +0200
Sam Ravnborg escreveu:
I'm already handling the other comments from your review (I'll send a
more complete comment about them after finishing), but I have a doubt
what you meant about this:
> +static int kirin_drm_bind(struct device *dev)
> > +{
> > + struct drm_driver *driver = _drm_driver;
> > + struct drm_device *drm_dev;
> > + struct kirin_drm_private *priv;
> > + int ret;
> > +
> > + drm_dev = drm_dev_alloc(driver, dev);
> > + if (!drm_dev)
> > + return -ENOMEM;
> > +
> > + ret = kirin_drm_kms_init(drm_dev);
> > + if (ret)
> > + goto err_drm_dev_unref;
> > +
> > + ret = drm_dev_register(drm_dev, 0);
> There is better ways to do this. See drm_drv.c for the code example.
Not sure if I understood your comment here. The drm_drv.c example also calls
drm_dev_register().
The only difference is that it calls it inside driver_probe(), while
on this driver, it uses:
static const struct component_master_ops kirin_drm_ops = {
.bind = kirin_drm_bind,
.unbind = kirin_drm_unbind,
};
static int kirin_drm_platform_probe(struct platform_device *pdev)
{
...
drm_of_component_match_add(dev, , compare_of, remote);
...
return component_master_add_with_match(dev, _drm_ops, match);
}
Are you meaning that I should get rid of this component API and call
kirin_drm_bind() from kirin_drm_platform_probe()?
Thanks,
Mauro
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] binder: print warnings when detecting oneway spamming.
The most common cause of the binder transaction buffer filling up is a
client rapidly firing oneway transactions into a process, before it has
a chance to handle them. Yet the root cause of this is often hard to
debug, because either the system or the app will stop, and by that time
binder debug information we dump in bugreports is no longer relevant.
This change warns as soon as a process dips below 80% of its oneway
space (less than 100kB available in the configuration), when any one
process is responsible for either more than 50 transactions, or more
than 50% of the oneway space.
Signed-off-by: Martijn Coenen
---
drivers/android/binder.c | 2 +-
drivers/android/binder_alloc.c | 49 +++---
drivers/android/binder_alloc.h | 5 +++-
3 files changed, 51 insertions(+), 5 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f936530a19b0..946332bc871a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3136,7 +3136,7 @@ static void binder_transaction(struct binder_proc *proc,
t->buffer = binder_alloc_new_buf(_proc->alloc, tr->data_size,
tr->offsets_size, extra_buffers_size,
- !reply && (t->flags & TF_ONE_WAY));
+ !reply && (t->flags & TF_ONE_WAY), current->tgid);
if (IS_ERR(t->buffer)) {
/*
* -ESRCH indicates VMA cleared. The target is dying.
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 69609696a843..76e8e633dbd4 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -338,12 +338,48 @@ static inline struct vm_area_struct *binder_alloc_get_vma(
return vma;
}
+static void debug_low_async_space_locked(struct binder_alloc *alloc, int pid)
+{
+ /*
+* Find the amount and size of buffers allocated by the current caller;
+* The idea is that once we cross the threshold, whoever is responsible
+* for the low async space is likely to try to send another async txn,
+* and at some point we'll catch them in the act. This is more efficient
+* than keeping a map per pid.
+*/
+ struct rb_node *n = alloc->free_buffers.rb_node;
+ struct binder_buffer *buffer;
+ size_t buffer_size;
+ size_t total_alloc_size = 0;
+ size_t num_buffers = 0;
+
+ for (n = rb_first(>allocated_buffers); n != NULL;
+n = rb_next(n)) {
+ buffer = rb_entry(n, struct binder_buffer, rb_node);
+ if (buffer->pid != pid)
+ continue;
+ if (!buffer->async_transaction)
+ continue;
+ buffer_size = binder_alloc_buffer_size(alloc, buffer);
+ total_alloc_size += buffer_size;
+ num_buffers++;
+ }
+
+ // Warn if this pid has more than 50% of async space, or more than 50
txns
+ if (num_buffers > 50 || total_alloc_size > alloc->buffer_size / 4) {
+ binder_alloc_debug(BINDER_DEBUG_USER_ERROR,
+"%d: pid %d spamming oneway? %zd buffers allocated
for a total size of %zd\n",
+ alloc->pid, pid, num_buffers, total_alloc_size);
+ }
+}
+
static struct binder_buffer *binder_alloc_new_buf_locked(
struct binder_alloc *alloc,
size_t data_size,
size_t offsets_size,
size_t extra_buffers_size,
- int is_async)
+ int is_async,
+ int pid)
{
struct rb_node *n = alloc->free_buffers.rb_node;
struct binder_buffer *buffer;
@@ -486,11 +522,16 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
buffer->offsets_size = offsets_size;
buffer->async_transaction = is_async;
buffer->extra_buffers_size = extra_buffers_size;
+ buffer->pid = pid;
if (is_async) {
alloc->free_async_space -= size + sizeof(struct binder_buffer);
binder_alloc_debug(BINDER_DEBUG_BUFFER_ALLOC_ASYNC,
"%d: binder_alloc_buf size %zd async free %zd\n",
alloc->pid, size, alloc->free_async_space);
+ if (alloc->free_async_space < alloc->buffer_size / 10) {
+ // Start detecting spammers once we reach 80% of async
space used
+ debug_low_async_space_locked(alloc, pid);
+ }
}
return buffer;
@@ -508,6 +549,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked(
* @offsets_size: user specified buffer offset
* @extra_buffers_size: size of extra space for meta-data (eg, security
context)
* @is_async: buffer for async transaction
+ * @pid: pid to
Re: [PATCH] binder: print warnings when detecting oneway spamming.
Hi Martijn,
I love your patch! Yet something to improve:
[auto build test ERROR on staging/staging-testing]
[also build test ERROR on v5.9-rc1 next-20200820]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Martijn-Coenen/binder-print-warnings-when-detecting-oneway-spamming/20200820-155358
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
bc752d2f345bf55d71b3422a6a24890ea03168dc
config: s390-randconfig-r002-20200818 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project
4deda57106f7c9b982a49cb907c33e3966c8de7f)
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
>> drivers/android/binder_alloc_selftest.c:122:61: error: too few arguments to
>> function call, expected 6, have 5
buffers[i] = binder_alloc_new_buf(alloc, sizes[i], 0, 0, 0);
^
drivers/android/binder_alloc.h:118:30: note: 'binder_alloc_new_buf' declared
here
extern struct binder_buffer *binder_alloc_new_buf(struct binder_alloc *alloc,
^
1 error generated.
#
https://github.com/0day-ci/linux/commit/9d0b269f4468d6793f6fd76a410fdde39dbf6ac2
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Martijn-Coenen/binder-print-warnings-when-detecting-oneway-spamming/20200820-155358
git checkout 9d0b269f4468d6793f6fd76a410fdde39dbf6ac2
vim +122 drivers/android/binder_alloc_selftest.c
4175e2b46fd4b9 Sherry Yang 2017-08-23 114
4175e2b46fd4b9 Sherry Yang 2017-08-23 115 static void
binder_selftest_alloc_buf(struct binder_alloc *alloc,
4175e2b46fd4b9 Sherry Yang 2017-08-23 116
struct binder_buffer *buffers[],
4175e2b46fd4b9 Sherry Yang 2017-08-23 117
size_t *sizes, int *seq)
4175e2b46fd4b9 Sherry Yang 2017-08-23 118 {
4175e2b46fd4b9 Sherry Yang 2017-08-23 119 int i;
4175e2b46fd4b9 Sherry Yang 2017-08-23 120
4175e2b46fd4b9 Sherry Yang 2017-08-23 121 for (i = 0; i < BUFFER_NUM;
i++) {
4175e2b46fd4b9 Sherry Yang 2017-08-23 @122 buffers[i] =
binder_alloc_new_buf(alloc, sizes[i], 0, 0, 0);
4175e2b46fd4b9 Sherry Yang 2017-08-23 123 if (IS_ERR(buffers[i])
||
4175e2b46fd4b9 Sherry Yang 2017-08-23 124
!check_buffer_pages_allocated(alloc, buffers[i],
4175e2b46fd4b9 Sherry Yang 2017-08-23 125
sizes[i])) {
4175e2b46fd4b9 Sherry Yang 2017-08-23 126
pr_err_size_seq(sizes, seq);
4175e2b46fd4b9 Sherry Yang 2017-08-23 127
binder_selftest_failures++;
4175e2b46fd4b9 Sherry Yang 2017-08-23 128 }
4175e2b46fd4b9 Sherry Yang 2017-08-23 129 }
4175e2b46fd4b9 Sherry Yang 2017-08-23 130 }
4175e2b46fd4b9 Sherry Yang 2017-08-23 131
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Mauro, On Thu, Aug 20, 2020 at 09:03:26AM +0200, Mauro Carvalho Chehab wrote: > Em Wed, 19 Aug 2020 12:52:06 -0700 John Stultz escreveu: > > On Wed, Aug 19, 2020 at 8:31 AM Laurent Pinchart wrote: > > > On Wed, Aug 19, 2020 at 05:21:20PM +0200, Sam Ravnborg wrote: > > > > On Wed, Aug 19, 2020 at 01:45:28PM +0200, Mauro Carvalho Chehab wrote: > > > > > This patch series port the out-of-tree driver for Hikey 970 (which > > > > > should also support Hikey 960) from the official 96boards tree: > > > > > > > > > >https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > > > > > > > > > > Based on his history, this driver seems to be originally written > > > > > for Kernel 4.4, and was later ported to Kernel 4.9. The original > > > > > driver used to depend on ION (from Kernel 4.4) and had its own > > > > > implementation for FB dev API. > > > > > > > > > > As I need to preserve the original history (with has patches from > > > > > both HiSilicon and from Linaro), I'm starting from the original > > > > > patch applied there. The remaining patches are incremental, > > > > > and port this driver to work with upstream Kernel. > > > > > > > ... > > > > > - Due to legal reasons, I need to preserve the authorship of > > > > > each one responsbile for each patch. So, I need to start from > > > > > the original patch from Kernel 4.4; > > ... > > > > I do acknowledge you need to preserve history and all - > > > > but this patchset is not easy to review. > > > > > > Why do we need to preserve history ? Adding relevant Signed-off-by and > > > Co-developed-by should be enough, shouldn't it ? Having a public branch > > > that contains the history is useful if anyone is interested, but I don't > > > think it's required in mainline. > > > > Yea. I concur with Laurent here. I'm not sure what legal reasoning you > > have on this but preserving the "absolute" history here is actively > > detrimental for review and understanding of the patch set. > > > > Preserving Authorship, Signed-off-by lines and adding Co-developed-by > > lines should be sufficient to provide both atribution credit and DCO > > history. > > I'm not convinced that, from legal standpoint, folding things would > be enough. See, there are at least 3 legal systems involved here > among the different patch authors: > > - civil law; > - common law; > - customary law + common law. > > Merging stuff altogether from different law systems can be problematic, > and trying to discuss this with experienced IP property lawyers will > for sure take a lot of time and efforts. I also bet that different > lawyers will have different opinions, because laws are subject to > interpretation. With that matter I'm not aware of any court rules > with regards to folded patches. So, it sounds to me that folding > patches is something that has yet to be proofed in courts around > the globe. > > At least for US legal system, it sounds that the Country of > origin of a patch is relevant, as they have a concept of > "national technology" that can be subject to export regulations. > > From my side, I really prefer to play safe and stay out of any such > legal discussions. Let's be serious for a moment. If you think there are legal issues in taking GPL-v2.0-only patches and squashing them while retaining authorship information through tags, the Linux kernel if *full* of that. You also routinely modify patches that you commit to the media subsystem to fix "small issues". The country of origin argument makes no sense either, the kernel code base if full of code coming from pretty much all country on the planet. Keeping the patches separate make this hard to review. Please squash them. -- Regards, Laurent Pinchart ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed
We added a config option CONFIG_TRACING_ALLOW_PRINTK to make sure
that no extra trace_printk gets added to the kernel, let's use
that in this driver to guard the trace_printk call.
Signed-off-by: Nicolas Boichat
---
Technically, we could only initialize the trace_printk buffers
when the print env is switched, to avoid the build error and
unconditional boot-time warning, but I assume this printing
framework will eventually get removed when the driver moves out
of staging?
drivers/staging/media/atomisp/pci/atomisp_compat_css20.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
index 1b2b2c68025b4cc..020519dca1324ab 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
@@ -165,11 +165,13 @@ static int atomisp_css2_dbg_print(const char *fmt,
va_list args)
return 0;
}
+#ifdef CONFIG_TRACING_ALLOW_PRINTK
static int atomisp_css2_dbg_ftrace_print(const char *fmt, va_list args)
{
ftrace_vprintk(fmt, args);
return 0;
}
+#endif
static int atomisp_css2_err_print(const char *fmt, va_list args)
{
@@ -865,9 +867,11 @@ static inline int __set_css_print_env(struct
atomisp_device *isp, int opt)
if (opt == 0)
isp->css_env.isp_css_env.print_env.debug_print = NULL;
+#ifdef CONFIG_TRACING_ALLOW_PRINTK
else if (opt == 1)
isp->css_env.isp_css_env.print_env.debug_print =
atomisp_css2_dbg_ftrace_print;
+#endif
else if (opt == 2)
isp->css_env.isp_css_env.print_env.debug_print =
atomisp_css2_dbg_print;
--
2.28.0.220.ged08abb693-goog
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 25/49] staging: hikey9xx/gpu: do some code cleanups
(added c/c Rob Herring)
Em Wed, 19 Aug 2020 18:53:06 -0700
John Stultz escreveu:
> On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab
> wrote:
> > @@ -376,7 +355,7 @@ static int kirin_drm_platform_resume(struct
> > platform_device *pdev)
> > }
> >
> > static const struct of_device_id kirin_drm_dt_ids[] = {
> > - { .compatible = "hisilicon,hi3660-dpe",
> > + { .compatible = "hisilicon,kirin960-dpe",
>
>
> One issue, elsewhere in your patch stack you still refer to the
> hisilicon,hi3660-dpe compatible string. This should probably be
> consistent one way or the other.
Agreed with regards to consistency.
It sounds to me that calling those as Kirin 9xx (and the previous one
as Kirin 620) is better than using the part number.
Here, googling for "Kirin 970" gave about 6.9 million hits, while "Hi3670"
gave only 75,5K hits.
Kirin 620 has similar results: 6.85 million hits, against 61,9 hits
for "Hi3620".
With "Kirin 960", the numbers are a lot higher: had 21,4 million hits,
against 423K hits for "Hi3660".
So, my preference is to use "Kirin 620, 960 and 970" for future changes.
-
Currently, there are already some inconsistency, as some places
use the part number where others use "Kirin xxx" designation,
when referring to Kirin 620, 960 and 970.
I would love to make this consistent among the Kernel. However,
I'm not sure if changing "compatible" would be acceptable
by DT maintainers.
If something like that would be OK, I can prepare a separate
patchset to be applied at the Kernel.
Thanks,
Mauro
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Wed, 19 Aug 2020 14:36:52 -0700 John Stultz escreveu: > On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab > wrote: > > So, IMO, the best is to keep it on staging for a while, until those > > remaining bugs gets solved. > > > > I added this series, together with the regulator driver and > > a few other patches (including a hack to fix a Kernel 5.8 > > regression at WiFi ) at: > > > > > > https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master > > Sorry, one more small request: Could you create a branch that only has > the DRM driver changes in it? > > The reason I ask, is that since the HiKey960 isn't affected by the > majority of the problems you listed as motivation for going through > staging. So if we can validate that your tree works fine on HiKey960, > the series can be cleaned up and submitted properly upstream to enable > that SoC, and the outstanding 970 issues can be worked out afterwards > against mainline. Well, if support for HiKey 960 is OK, I guess what we can do is to not push the patch with DT bindings for hikey970. We should probably fix the color swap thing at the driver first. >From my side, provided that the history is preserved, I don't mind if this is merged: - via staging tree; - at dri-devel tree; - or having a the historic patchsets merged at /staging, with a follow up patch moving it from staging/ into /gpu/drm/. Thanks, Mauro ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH][next] staging: spmi: hisi-spmi-controller: fix spelling mistake "controlller" -> "controller"
From: Colin Ian King
There is a spelling mistake in the MODULE_ALIAS, fix it.
Signed-off-by: Colin Ian King
---
drivers/staging/hikey9xx/hisi-spmi-controller.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/hikey9xx/hisi-spmi-controller.c
b/drivers/staging/hikey9xx/hisi-spmi-controller.c
index 66a0b296e06f..b132b2a5d939 100644
--- a/drivers/staging/hikey9xx/hisi-spmi-controller.c
+++ b/drivers/staging/hikey9xx/hisi-spmi-controller.c
@@ -354,4 +354,4 @@ module_exit(spmi_controller_exit);
MODULE_LICENSE("GPL v2");
MODULE_VERSION("1.0");
-MODULE_ALIAS("platform:spmi_controlller");
+MODULE_ALIAS("platform:spmi_controller");
--
2.27.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Wed, 19 Aug 2020 20:28:44 -0700 John Stultz escreveu: > On Wed, Aug 19, 2020 at 7:01 PM John Stultz wrote: > > > > On Wed, Aug 19, 2020 at 2:36 PM John Stultz wrote: > > > > > > > > On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab > > > wrote: > > > > So, IMO, the best is to keep it on staging for a while, until those > > > > remaining bugs gets solved. > > > > > > > > I added this series, together with the regulator driver and > > > > a few other patches (including a hack to fix a Kernel 5.8 > > > > regression at WiFi ) at: > > > > > > > > > > > > https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master > > > > > > > > > > Sorry, one more small request: Could you create a branch that only has > > > the DRM driver changes in it? > > > > > > The reason I ask, is that since the HiKey960 isn't affected by the > > > majority of the problems you listed as motivation for going through > > > staging. So if we can validate that your tree works fine on HiKey960, > > > the series can be cleaned up and submitted properly upstream to enable > > > that SoC, and the outstanding 970 issues can be worked out afterwards > > > against mainline. > > > > Just as a heads up, I tried testing your tree with my HiKey960, and > > after fixing the compat string inconsistency, the drivers seem to load > > properly. However the drm_hwcomposer seems to have some trouble with > > the driver: > > 01-01 00:12:41.456 345 345 E hwc-drm-display-compositor: Commit > > test failed for display 0, FIXME > > 01-01 00:12:41.456 345 345 E hwc-drm-two: Failed to apply the > > frame composition ret=-22 > > 01-01 00:12:41.456 351 351 E HWComposer: > > presentAndGetReleaseFences: present failed for display 0: BadParameter > > (4) > > > > I'll dig in a bit further as to why, but wanted to give you a heads up. > > Ok, I've mostly gotten it sorted out: > - You're missing a few color formats. > - And I re-discovered a crash that was already fixed in my tree. > > I'll send those patches in a few here. Thank you for the patches! I'll test them with Hikey 970 in order to be sure they're compatible also with such SoC. > > That said even with the patches I've got on top of your series, I > still see a few issues: > 1) I'm seeing red-blue swap with your driver. I need to dig a bit to > see what the difference is, I know gralloc has a config option for > this, and maybe the version of the driver I'm carrying has it wrong? There are some settings at adv7535 with regards to the colormap. The 4.9 fork of it has some different settings. Maybe it could be somehow related to it. I have here a Hikey 960, but didn't test it yet. > 2) Performance is noticeably worse. Whereas with my tree, I see close > to 60fps (that clk issue we mentioned earlier is why it's not exactly > 60) in most tests, but with yours it mostly hovers around 30some fps, > occasionally speeding up to 40 and then back down. That's weird, but it could be due to some settings related to CMA, IOMMU and/or AFBC. > Obviously with some work I suspect we'll be able to sort these out, > but I also do feel that the set you're starting with for upstreaming > is pretty old. The driver I'm carrying was heavily refactored around > 5.0 to share code with the existing kirin driver, in the hopes of > making usptreaming easier, and it seems a shame to throw that out and > focus your efforts on the older tree. > > But to be fair, I've not had time to upstream the driver myself, and > it's obviously your choice on how you spend your time. I am really > excited to see your efforts here, regardless of which driver you end > up pushing. On a quick look I've done, besides not having support for Hikey 970, the code on your tree seems to have less settings than the original one for Hikey 960. Yet, it should take some time to figure out what those extra settings are doing. Once I get this driver merged, and have USB support working fine[1], my plan is to compare the version from your tree, and compare with the one I have, in order to cleanup some stuff, check performance and do some other optimizations. - [1] this is a little OOT here: USB has been a challenge. Depending on the build, I'm getting an NMI interrupt error when the USB3 stack is loaded (usually at dwc3). The error is ESR_ELx_AET_UC. Unfortunately, it doesn't point to where this error is generated, making very hard to debug it. Thanks, Mauro ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
On 2020-08-19 10:48 p.m., Sam Ravnborg wrote: > Hi Mauro. > > It seems my review comments failed to reach dri-devel - likely due to > the size of the mail. Right, some e-mails in this thread went through the dri-devel moderation queue due to their sizes. This mail of yours did as well, because you didn't trim the quoted text (hint, hint). -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Hi Sam, Em Wed, 19 Aug 2020 22:48:00 +0200 Sam Ravnborg escreveu: > Hi Mauro. > > It seems my review comments failed to reach dri-devel - likely due to > the size of the mail. Probably. It reached here properly. > Link: > https://lore.kernel.org/linux-devicetree/20200819173558.ga3...@ravnborg.org/ > > I my review feedback I refer to checkpatch a few time. > For drivers/gpu/ we have some nice tooling support. > One thing our tooling does for us is running checkpatch every time > we apply a patch. > > checkpatch -q --emacs --strict --show-types > > So we expect patches to be more or less checkpatch --strict clean. > > "more or less" - as common sense also plays a role. > And sometimes checkpatch is just wrong. > > Just in case you wondered why checkpatch --strict was requested. We also use checkpatch --strict for media as a reference, ignoring the things that would make things worse during review :-) I'll run checkpatch here and ensure that the coding style issues will be properly addressed. Thanks, Mauro ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Wed, 19 Aug 2020 12:52:06 -0700 John Stultz escreveu: > On Wed, Aug 19, 2020 at 8:31 AM Laurent Pinchart > wrote: > > On Wed, Aug 19, 2020 at 05:21:20PM +0200, Sam Ravnborg wrote: > > > On Wed, Aug 19, 2020 at 01:45:28PM +0200, Mauro Carvalho Chehab wrote: > > > > This patch series port the out-of-tree driver for Hikey 970 (which > > > > should also support Hikey 960) from the official 96boards tree: > > > > > > > >https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > > > > > > > > Based on his history, this driver seems to be originally written > > > > for Kernel 4.4, and was later ported to Kernel 4.9. The original > > > > driver used to depend on ION (from Kernel 4.4) and had its own > > > > implementation for FB dev API. > > > > > > > > As I need to preserve the original history (with has patches from > > > > both HiSilicon and from Linaro), I'm starting from the original > > > > patch applied there. The remaining patches are incremental, > > > > and port this driver to work with upstream Kernel. > > > > > ... > > > > - Due to legal reasons, I need to preserve the authorship of > > > > each one responsbile for each patch. So, I need to start from > > > > the original patch from Kernel 4.4; > ... > > > I do acknowledge you need to preserve history and all - > > > but this patchset is not easy to review. > > > > Why do we need to preserve history ? Adding relevant Signed-off-by and > > Co-developed-by should be enough, shouldn't it ? Having a public branch > > that contains the history is useful if anyone is interested, but I don't > > think it's required in mainline. > > Yea. I concur with Laurent here. I'm not sure what legal reasoning you > have on this but preserving the "absolute" history here is actively > detrimental for review and understanding of the patch set. > > Preserving Authorship, Signed-off-by lines and adding Co-developed-by > lines should be sufficient to provide both atribution credit and DCO > history. I'm not convinced that, from legal standpoint, folding things would be enough. See, there are at least 3 legal systems involved here among the different patch authors: - civil law; - common law; - customary law + common law. Merging stuff altogether from different law systems can be problematic, and trying to discuss this with experienced IP property lawyers will for sure take a lot of time and efforts. I also bet that different lawyers will have different opinions, because laws are subject to interpretation. With that matter I'm not aware of any court rules with regards to folded patches. So, it sounds to me that folding patches is something that has yet to be proofed in courts around the globe. At least for US legal system, it sounds that the Country of origin of a patch is relevant, as they have a concept of "national technology" that can be subject to export regulations. >From my side, I really prefer to play safe and stay out of any such legal discussions. Thanks, Mauro ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Wed, 19 Aug 2020 23:25:51 +0200 Sam Ravnborg escreveu: > Hi John. > > > > So, IMO, the best is to keep it on staging for a while, until those > > > remaining bugs gets solved. > > > > I'm not sure I see all of these as compelling for pushing it in via > > staging. And I suspect in the process of submitting the patches for > > review folks may find the cause of some of the problems you list here. > > There is a tendency to forget drivers in staging, and with the almost > constant refactoring that happens in the drm drivers we would end up > fixing this driver when a bot trigger an error. > So IMO we need very good reasons to go in via staging. My plan is to have this driver upstream for 5.10, and getting it out of staging by Kernel 5.11. So, I doubt that the DRM kAPIs would change a lot during those 2 Kernel cycles. In any case, I'm also fine to have a final patch at the end of this series moving it out of staging. The only thing that, IMHO, prevents it to be out of staging is the LDI underflow. Right now, if no input events reach the driver, DPMS will put the monitor to suspend, and it never returns back from life. I bet that, once we discover the root cause, the fix would be just a couple of lines, but identifying where the problem is can take a while. Thanks, Mauro ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
Em Wed, 19 Aug 2020 14:13:05 -0700 John Stultz escreveu: > On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab > wrote: > > Yet, I'm submitting it via staging due to the following reasons: > > > > - It depends on the LDO3 power supply, which is provided by > > a regulator driver that it is currently on staging; > > - Due to legal reasons, I need to preserve the authorship of > > each one responsbile for each patch. So, I need to start from > > the original patch from Kernel 4.4; > > - There are still some problems I need to figure out how to solve: > >- The adv7535 can't get EDID data. Maybe it is a timing issue, > > but it requires more research to be sure about how to solve it; > > I've seen this on the HiKey960 as well. There is a patch to the > adv7533 driver I have to add a mdelay that seems to consistently > resolve the timing problem. At some point I mentioned it to one of > the maintainers who seems open to having it added, but it seemed silly > to submit it until there was a upstream driver that needed such a > change. So I think that patch can be submitted as a follow on to this > (hopefully cleaned up) series. Yeah, I saw that mdelay() patch on your tree. While it should be cheap to add a mdelay() or udelay_range() there, I'm not sure if this is just a timing issue or if something else is missing. The 4.9 driver does some extra setups on adv7535 (see the enclosed patch). I'm wondering if something from that is missing. Btw, IMHO, one interesting setting on the downstream code is support for colorbar test. This was helpful when I was making this driver work upstream, as it could be useful for someone trying to make some other DRM driver using it. > > >- The driver only accept resolutions on a defined list, as there's > > a known bug that this driver may have troubles with random > > resolutions. Probably due to a bug at the pixel clock settings; > > So, yes, the SoC clks can't generate proper signals for HDMI > frequencies (apparently it's not an issue for panels). There is a > fixed set that we can get "close enough" that most monitors will work, > but its always a bit iffy (some monitors are strict in what they > take). There is an extra logic for Kirin 620 that seems to be validating the frequencies that would be set to the clocks. If they're out of the range, it would return MODE_BAD. I suspect that something similar would be needed for Kirin 970 and 960. With that regards, 970 is different from 960. I actually tried to write a patch for it, but I guess there are still some things missing on my code, for 970. This patch: https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commit/4614415770b33e27a9f15c7dde20895fb750592f Splits the part of the code which calculates the clk_Hz from the part which sets it. So, now, dss_calculate_clock() checks the pixel clock frequency and adjusts it, if needed. I have another patch that were checking if the code modified the clock_Hz at dss_crtc_mode_fixup(). With that, it would be easy to return MODE_INVALID if something bad happens. However, such patch didn't work as I would expect, so I ended dropping it. > On the kirin driver, we were able to do a calculation to figure out if > the generated frequency would be close enough: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c#n615 > > I suspect we could do something similar for the hikey960/70, but I've > not really had time to dig in deeply there. Yeah, I tried something similar to that. Maybe it will work properly for Hikey 960, but Hikey 970 has a much more complex code to setup the pixel clock. On 960, just one clock is set: clk_set_rate(ctx->dss_pxl0_clk, clk_Hz); While, on 970, pxl0 is always set to the same frequency (144 MHz), at least for pixel clocks up to 255 MHz[1], but the code do some complex calculus to setup PLL7 registers using the clock frequency. [1] pixel clocks above 255 MHz will be rejected anyway, as they'll return MODE_BAD due to the checks if a mode is valid. > > Personally, I don't see the allow-list as a problematic short term > solution, and again, not sure its worth pushing to staging for. Yeah, I guess we can live with that for a while. > > >- Sometimes (at least with 1080p), it generates LDI underflow > > errors, which in turn causes the DRM to stop working. That > > happens for example when using gdm on Wayland and > > gnome on X11; > > Interestingly, I've not seen this on HiKey960 (at least with > Android/Surfaceflinger). Here, it happens all the time when the monitor returns back from DPMS suspend. It also happens on some specific cases (X11 x Wayland), console mode + startx, etc. It sounds to me that it occurs when the driver tries to setup a new resolution. Maybe something needs to be disabled before changing res (and re-enabled afterwards). > The original HiKey board does have the > trouble
