Re: [PATCH v6 2/4] dax/bus: Use guard(device) in sysfs attribute helpers
On Fri, Dec 15, 2023 at 05:32:50PM +, Verma, Vishal L wrote:
> On Fri, 2023-12-15 at 09:15 -0800, Dan Williams wrote:
> > Greg Kroah-Hartman wrote:
> > > On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote:
> > > > Use the guard(device) macro to lock a 'struct device', and unlock it
> > > > automatically when going out of scope using Scope Based Resource
> > > > Management semantics. A lot of the sysfs attribute writes in
> > > > drivers/dax/bus.c benefit from a cleanup using these, so change these
> > > > where applicable.
> > >
> > > Wait, why are you needing to call device_lock() at all here? Why is dax
> > > special in needing this when no other subsystem requires it?
> > >
> > > >
> > > > Cc: Joao Martins
> > > > Cc: Dan Williams
> > > > Signed-off-by: Vishal Verma
> > > > ---
> > > > drivers/dax/bus.c | 143
> > > > ++
> > > > 1 file changed, 59 insertions(+), 84 deletions(-)
> > > >
> > > > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> > > > index 1ff1ab5fa105..6226de131d17 100644
> > > > --- a/drivers/dax/bus.c
> > > > +++ b/drivers/dax/bus.c
> > > > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device
> > > > *dev,
> > > > struct device_attribute *attr, char *buf)
> > > > {
> > > > struct dax_region *dax_region = dev_get_drvdata(dev);
> > > > - unsigned long long size;
> > > >
> > > > - device_lock(dev);
> > > > - size = dax_region_avail_size(dax_region);
> > > > - device_unlock(dev);
> > > > + guard(device)(dev);
> > >
> > > You have a valid device here, why are you locking it? How can it go
> > > away? And if it can, shouldn't you have a local lock for it, and not
> > > abuse the driver core lock?
> >
> > Yes, this is a driver-core lock abuse written by someone who should have
> > known better. And yes, a local lock to protect the dax_region resource
> > tree should replace this. A new rwsem to synchronize all list walks
> > seems appropriate.
>
> I see why _a_ lock is needed both here and in size_show() - the size
> calculations do a walk over discontiguous ranges, and we don't want the
> device to get reconfigured in the middle of that. A different local
> lock seems reasonable - however can that go as a separate cleanup that
> stands on its own?
Sure, but do not add a conversion to use guard(device) here, as that
will be pointless as you will just use a real lock instead.
> For this series, I'll add a cleanup to replace the sprintfs with
> sysfs_emit().
Why not have that be the first patch in the series? Then add your local
lock and convert everything to use it instead of the device lock?
thanks,
greg k-h
Re: [PATCH v6 2/4] dax/bus: Use guard(device) in sysfs attribute helpers
On Fri, Dec 15, 2023 at 06:33:58AM +, Verma, Vishal L wrote:
> On Fri, 2023-12-15 at 05:56 +, Matthew Wilcox wrote:
> > On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote:
> > > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device
> > > *dev,
> > > struct device_attribute *attr, char *buf)
> > > {
> > > struct dax_region *dax_region = dev_get_drvdata(dev);
> > > - unsigned long long size;
> > >
> > > - device_lock(dev);
> > > - size = dax_region_avail_size(dax_region);
> > > - device_unlock(dev);
> > > + guard(device)(dev);
> > >
> > > - return sprintf(buf, "%llu\n", size);
> > > + return sprintf(buf, "%llu\n", dax_region_avail_size(dax_region));
> > > }
> >
> > Is this an appropriate use of guard()? sprintf is not the fastest of
> > functions, so we will end up holding the device_lock for longer than
> > we used to.
>
> Hi Matthew,
>
> Agreed that we end up holding the lock for a bit longer in many of
> these. I'm inclined to say this is okay, since these are all user
> configuration paths through sysfs, not affecting any sort of runtime
> performance.
Why does the lock have to be taken at all? You have a valid reference,
isn't that all you need?
thanks,
greg k-h
Re: [PATCH 5.4 v2 1/1] iommu/vt-d: Fix agaw for a supported 48 bit guest address width
On Mon, Apr 12, 2021 at 06:25:44PM +, Saeed Mirzamohammadi wrote: > Hi Greg, > > This patch fixes an issue with the IOMMU driver and it only applies to 5.4, > 4.19, and 4.14 stable kernels. May I know when this patch would be available > in the stable kernels? > > Subject: iommu/vt-d: Fix agaw for a supported 48 bit guest address width > This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.
Re: [PATCH 1/2] extcon: extcon-gpio: Log error if work-queue init fails
On Thu, Mar 25, 2021 at 04:52:12AM +, Vaittinen, Matti wrote:
>
> On Thu, 2021-03-25 at 09:49 +0900, Chanwoo Choi wrote:
> > On 3/24/21 6:51 PM, Vaittinen, Matti wrote:
> > > Hello Hans, Chanwoo, Greg,
> > >
> > > On Wed, 2021-03-24 at 10:25 +0100, Hans de Goede wrote:
> > > > Hi,
> > > >
> > > > On 3/24/21 10:21 AM, Matti Vaittinen wrote:
> > > > > Add error print for probe failure when resource managed work-
> > > > > queue
> > > > > initialization fails.
> > > > >
> > > > > Signed-off-by: Matti Vaittinen <
> > > > > matti.vaitti...@fi.rohmeurope.com>
> > > > > Suggested-by: Chanwoo Choi
> > > > > ---
> > > > > drivers/extcon/extcon-gpio.c | 4 +++-
> > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/extcon/extcon-gpio.c
> > > > > b/drivers/extcon/extcon-
> > > > > gpio.c
> > > > > index 4105df74f2b0..8ea2cda8f7f3 100644
> > > > > --- a/drivers/extcon/extcon-gpio.c
> > > > > +++ b/drivers/extcon/extcon-gpio.c
> > > > > @@ -114,8 +114,10 @@ static int gpio_extcon_probe(struct
> > > > > platform_device *pdev)
> > > > > return ret;
> > > > >
> > > > > ret = devm_delayed_work_autocancel(dev, >work,
> > > > > gpio_extcon_work);
> > > > > - if (ret)
> > > > > + if (ret) {
> > > > > + dev_err(dev, "Failed to initialize
> > > > > delayed_work");
> > > > > return ret;
> > > > > + }
> > > >
> > > > The only ret which we can have here is -ENOMEM and as a rule we
> > > > don't
> > > > log
> > > > errors for those, because the kernel memory-management code
> > > > already
> > > > complains
> > > > loudly when this happens.
> > >
> > > I know. This is why I originally omitted the print. Besides, if the
> > > memory is so low that devres adding fails - then we probably have
> > > plenty of other complaints as well... But as Chanwoo maintains the
> > > driver and wanted to have the print - I do not have objections to
> > > that
> > > either. Maybe someone some-day adds another error path to wq
> > > initialization in which case seeing it failed could make sense.
> > >
> > > > So IMHO this patch should be dropped.
> > > Fine for me - as well as keeping it. I have no strong opinion on
> > > this.
> >
> > If it is the same handling way for -ENOMEM, don't need to add log ss
> > Hans said.
> > Thanks for Hans.
>
> So be it :)
> Greg, can you just apply the patch 2/2 and drop this one? (There should
> be no dependency between these) or do you want me to resend 2/2 alone?
I'll just take the 2/2 patch, thanks.
greg k-h
Re: [PATCH 5.10 113/290] net: dsa: implement a central TX reallocation procedure
On Tue, Mar 16, 2021 at 09:54:01AM -0400, Sasha Levin wrote:
> On Tue, Mar 16, 2021 at 06:46:10AM +0100, gre...@linuxfoundation.org wrote:
> > On Mon, Mar 15, 2021 at 07:56:02PM +, Vladimir Oltean wrote:
> > > +Andrew, Vivien,
> > >
> > > On Mon, Mar 15, 2021 at 02:53:26PM +0100, gre...@linuxfoundation.org
> > > wrote:
> > > > From: Greg Kroah-Hartman
> > > >
> > > > From: Vladimir Oltean
> > > >
> > > > [ Upstream commit a3b0b6479700a5b0af2c631cb2ec0fb7a0d978f2 ]
> > > >
> > > > At the moment, taggers are left with the task of ensuring that the skb
> > > > headers are writable (which they aren't, if the frames were cloned for
> > > > TX timestamping, for flooding by the bridge, etc), and that there is
> > > > enough space in the skb data area for the DSA tag to be pushed.
> > > >
> > > > Moreover, the life of tail taggers is even harder, because they need to
> > > > ensure that short frames have enough padding, a problem that normal
> > > > taggers don't have.
> > > >
> > > > The principle of the DSA framework is that everything except for the
> > > > most intimate hardware specifics (like in this case, the actual packing
> > > > of the DSA tag bits) should be done inside the core, to avoid having
> > > > code paths that are very rarely tested.
> > > >
> > > > So provide a TX reallocation procedure that should cover the known needs
> > > > of DSA today.
> > > >
> > > > Note that this patch also gives the network stack a good hint about the
> > > > headroom/tailroom it's going to need. Up till now it wasn't doing that.
> > > > So the reallocation procedure should really be there only for the
> > > > exceptional cases, and for cloned packets which need to be unshared.
> > > >
> > > > Signed-off-by: Vladimir Oltean
> > > > Tested-by: Christian Eggers # For tail taggers only
> > > > Tested-by: Kurt Kanzenbach
> > > > Reviewed-by: Florian Fainelli
> > > > Signed-off-by: Jakub Kicinski
> > > > Signed-off-by: Sasha Levin
> > > > ---
> > >
> > > For context, Sasha explains here:
> > > https://www.spinics.net/lists/stable-commits/msg190151.html
> > > (the conversation is somewhat truncated, unfortunately, because
> > > stable-comm...@vger.kernel.org ate my replies)
> > > that 13 patches were backported to get the unrelated commit 9200f515c41f
> > > ("net: dsa: tag_mtk: fix 802.1ad VLAN egress") to apply cleanly with
> > > git-am.
> > >
> > > I am not strictly against this, even though I would have liked to know
> > > that the maintainers were explicitly informed about it.
> > >
> > > Greg, could you make your stable backporting emails include the output
> > > of ./get_maintainer.pl into the list of recipients? Thanks.
> >
> > I cc: everyone on the signed-off-by list on the patch, why would we need
> > to add more? A maintainer should always be on that list automatically.
>
> Oh, hm, could this be an issue with subsystems that have a shared
> maintainership model? In that scenario not all maintainers will sign-off
> on a commit.
So a shared maintainer trusts their co-maintainer for reviewing patches
for Linus's tree and all future kernels, but NOT into an old backported
stable tree? I doubt that, trust should be the same for both.
thanks,
greg k-h
Re: [PATCH 5.10 113/290] net: dsa: implement a central TX reallocation procedure
On Mon, Mar 15, 2021 at 05:06:16PM -0400, Sasha Levin wrote:
> On Mon, Mar 15, 2021 at 07:56:02PM +, Vladimir Oltean wrote:
> > > Signed-off-by: Vladimir Oltean
> > > Tested-by: Christian Eggers # For tail taggers only
> > > Tested-by: Kurt Kanzenbach
> > > Reviewed-by: Florian Fainelli
> > > Signed-off-by: Jakub Kicinski
> > > Signed-off-by: Sasha Levin
> > > ---
> >
> > For context, Sasha explains here:
> > https://www.spinics.net/lists/stable-commits/msg190151.html
> > (the conversation is somewhat truncated, unfortunately, because
> > stable-comm...@vger.kernel.org ate my replies)
> > that 13 patches were backported to get the unrelated commit 9200f515c41f
> > ("net: dsa: tag_mtk: fix 802.1ad VLAN egress") to apply cleanly with git-am.
> >
> > I am not strictly against this, even though I would have liked to know
> > that the maintainers were explicitly informed about it.
> >
> > Greg, could you make your stable backporting emails include the output
> > of ./get_maintainer.pl into the list of recipients? Thanks.
>
> Did it not happen here? I've looked at Greg's script[1] and it seemed to
> me like it does go through get_maintainer.pl.
>
>
> [1]
> https://github.com/gregkh/gregkh-linux/blob/master/scripts/generate_cc_list
That's just a script I use for "normal" kernel development when creating
patches, not for stable stuff.
thanks,
greg k-h
Re: [PATCH 5.10 113/290] net: dsa: implement a central TX reallocation procedure
On Mon, Mar 15, 2021 at 07:56:02PM +, Vladimir Oltean wrote:
> +Andrew, Vivien,
>
> On Mon, Mar 15, 2021 at 02:53:26PM +0100, gre...@linuxfoundation.org wrote:
> > From: Greg Kroah-Hartman
> >
> > From: Vladimir Oltean
> >
> > [ Upstream commit a3b0b6479700a5b0af2c631cb2ec0fb7a0d978f2 ]
> >
> > At the moment, taggers are left with the task of ensuring that the skb
> > headers are writable (which they aren't, if the frames were cloned for
> > TX timestamping, for flooding by the bridge, etc), and that there is
> > enough space in the skb data area for the DSA tag to be pushed.
> >
> > Moreover, the life of tail taggers is even harder, because they need to
> > ensure that short frames have enough padding, a problem that normal
> > taggers don't have.
> >
> > The principle of the DSA framework is that everything except for the
> > most intimate hardware specifics (like in this case, the actual packing
> > of the DSA tag bits) should be done inside the core, to avoid having
> > code paths that are very rarely tested.
> >
> > So provide a TX reallocation procedure that should cover the known needs
> > of DSA today.
> >
> > Note that this patch also gives the network stack a good hint about the
> > headroom/tailroom it's going to need. Up till now it wasn't doing that.
> > So the reallocation procedure should really be there only for the
> > exceptional cases, and for cloned packets which need to be unshared.
> >
> > Signed-off-by: Vladimir Oltean
> > Tested-by: Christian Eggers # For tail taggers only
> > Tested-by: Kurt Kanzenbach
> > Reviewed-by: Florian Fainelli
> > Signed-off-by: Jakub Kicinski
> > Signed-off-by: Sasha Levin
> > ---
>
> For context, Sasha explains here:
> https://www.spinics.net/lists/stable-commits/msg190151.html
> (the conversation is somewhat truncated, unfortunately, because
> stable-comm...@vger.kernel.org ate my replies)
> that 13 patches were backported to get the unrelated commit 9200f515c41f
> ("net: dsa: tag_mtk: fix 802.1ad VLAN egress") to apply cleanly with git-am.
>
> I am not strictly against this, even though I would have liked to know
> that the maintainers were explicitly informed about it.
>
> Greg, could you make your stable backporting emails include the output
> of ./get_maintainer.pl into the list of recipients? Thanks.
I cc: everyone on the signed-off-by list on the patch, why would we need
to add more? A maintainer should always be on that list automatically.
thanks,
greg k-h
Re: [PATCH] e1000e: use proper #include guard name in hw.h
On Tue, Mar 02, 2021 at 01:37:59AM +, Nguyen, Anthony L wrote: > On Sat, 2021-02-27 at 10:58 +0100, Greg Kroah-Hartman wrote: > > The include guard for the e1000e and e1000 hw.h files are the same, > > so > > add the proper "E" term to the hw.h file for the e1000e driver. > > There's a patch in process that addresses this issue [1]. Thanks, hopefully it gets fixed somehow :) greg k-h
Re: [PATCH v10 01/20] dlb: add skeleton for DLB driver
On Thu, Feb 18, 2021 at 07:34:31AM +, Chen, Mike Ximing wrote: > > > > -Original Message- > > From: Mike Ximing Chen > > Sent: Wednesday, February 10, 2021 12:54 PM > > To: net...@vger.kernel.org > > Cc: da...@davemloft.net; k...@kernel.org; a...@arndb.de; > > gre...@linuxfoundation.org; Williams, Dan J ; > > pierre- > > louis.boss...@linux.intel.com; Gage Eads > > Subject: [PATCH v10 01/20] dlb: add skeleton for DLB driver > > > > diff --git a/Documentation/misc-devices/dlb.rst b/Documentation/misc- > > devices/dlb.rst > > new file mode 100644 > > index ..aa79be07ee49 > > --- /dev/null > > +++ b/Documentation/misc-devices/dlb.rst > > @@ -0,0 +1,259 @@ > > +.. SPDX-License-Identifier: GPL-2.0-only > > + > > +=== > > +Intel(R) Dynamic Load Balancer Overview > > +=== > > + > > +:Authors: Gage Eads and Mike Ximing Chen > > + > > +Contents > > + > > + > > +- Introduction > > +- Scheduling > > +- Queue Entry > > +- Port > > +- Queue > > +- Credits > > +- Scheduling Domain > > +- Interrupts > > +- Power Management > > +- User Interface > > +- Reset > > + > > +Introduction > > + > > + > > +The Intel(r) Dynamic Load Balancer (Intel(r) DLB) is a PCIe device that > > +provides load-balanced, prioritized scheduling of core-to-core > > communication. > > + > > +Intel DLB is an accelerator for the event-driven programming model of > > +DPDK's Event Device Library[2]. The library is used in packet processing > > +pipelines that arrange for multi-core scalability, dynamic load-balancing, > > and > > +variety of packet distribution and synchronization schemes. > > + > > +Intel DLB device consists of queues and arbiters that connect producer > > +cores and consumer cores. The device implements load-balanced queueing > > features > > +including: > > +- Lock-free multi-producer/multi-consumer operation. > > +- Multiple priority levels for varying traffic types. > > +- 'Direct' traffic (i.e. multi-producer/single-consumer) > > +- Simple unordered load-balanced distribution. > > +- Atomic lock free load balancing across multiple consumers. > > +- Queue element reordering feature allowing ordered load-balanced > > distribution. > > + > > Hi Jakub/Dave, > This is a device driver for a HW core-to-core communication accelerator. It > is submitted > to "linux-kernel" for a module under device/misc. Greg suggested (see below) > that we > also sent it to you for any potential feedback in case there is any > interaction with > networking initiatives. The device is used to handle the load balancing among > CPU cores > after the packets are received and forwarded to CPU. We don't think it > interferes > with networking operations, but would appreciate very much your > review/comment on this. It's the middle of the merge window, getting maintainers to review new stuff until after 5.12-rc1 is out is going to be a very difficult thing to do. In the meantime, why don't you all help out and review submitted patches to the mailing lists for the subsystems you all are trying to get this patch into. I know maintainers would appreciate the help, right? thanks, greg k-h
Re: [PATCH v2] vfs: prevent copy_file_range to copy across devices
On Wed, Feb 17, 2021 at 05:50:35PM -0700, Andreas Dilger wrote:
> On Feb 17, 2021, at 1:08 AM, Amir Goldstein wrote:
> >
> > You are missing my point.
> > Never mind which server. The server does not *need* to rely on
> > vfs_copy_file_range() to copy files from XFS to ext4.
> > The server is very capable of implementing the fallback generic copy
> > in case source/target fs do not support native {copy,remap}_file_range().
> >
> > w.r.t semantics of copy_file_range() syscall vs. the fallback to userespace
> > 'cp' tool (check source file size before copy or not), please note that the
> > semantics of CIFS_IOC_COPYCHUNK_FILE are that of the former:
> >
> >rc = cifs_file_copychunk_range(xid, src_file.file, 0, dst_file, 0,
> >src_inode->i_size, 0);
> >
> > It will copy zero bytes if advertised source file size if zero.
> >
> > NFS server side copy semantics are currently de-facto the same
> > because both the client and the server will have to pass through this
> > line in vfs_copy_file_range():
> >
> >if (len == 0)
> >return 0;
> >
> > IMO, and this opinion was voiced by several other filesystem developers,
> > the shortend copy semantics are the correct semantics for copy_file_range()
> > syscall as well as for vfs_copy_file_range() for internal kernel users.
> >
> > I guess what this means is that if the 'cp' tool ever tries an opportunistic
> > copy_file_range() syscall (e.g. --cfr=auto), it may result in zero size
> > copy.
>
> Having a syscall that does the "wrong thing" when called on two files
> doesn't make sense. Expecting userspace to check whether source/target
> files supports CFR is also not practical. This is trivial for the
> kernel to determine and return -EOPNOTSUPP to the caller if the source
> file (procfs/sysfs/etc) does not work with CFR properly.
How does the kernel "know" that a specific file in a specific filesystem
will not work with CFR "properly"? That goes back to the original patch
which tried to label each and every filesystem type with a
"supported/not supported" type of flag, which was going to be a mess,
especially as it seems that this might be a file-specific thing, not a
filesystem-specific thing.
The goal of the patch _should_ be that the kernel figure it out itself,
but so far no one seems to be able to explain how that can be done :(
So, any hints?
thanks,
greg k-h
Re: [PATCH v2] vfs: prevent copy_file_range to copy across devices
On Tue, Feb 16, 2021 at 11:17:34AM +, Luis Henriques wrote:
> Amir Goldstein writes:
>
> > On Mon, Feb 15, 2021 at 8:57 PM Trond Myklebust
> > wrote:
> >>
> >> On Mon, 2021-02-15 at 19:24 +0200, Amir Goldstein wrote:
> >> > On Mon, Feb 15, 2021 at 6:53 PM Trond Myklebust <
> >> > tron...@hammerspace.com> wrote:
> >> > >
> >> > > On Mon, 2021-02-15 at 18:34 +0200, Amir Goldstein wrote:
> >> > > > On Mon, Feb 15, 2021 at 5:42 PM Luis Henriques <
> >> > > > lhenriq...@suse.de>
> >> > > > wrote:
> >> > > > >
> >> > > > > Nicolas Boichat reported an issue when trying to use the
> >> > > > > copy_file_range
> >> > > > > syscall on a tracefs file. It failed silently because the file
> >> > > > > content is
> >> > > > > generated on-the-fly (reporting a size of zero) and
> >> > > > > copy_file_range
> >> > > > > needs
> >> > > > > to know in advance how much data is present.
> >> > > > >
> >> > > > > This commit restores the cross-fs restrictions that existed
> >> > > > > prior
> >> > > > > to
> >> > > > > 5dae222a5ff0 ("vfs: allow copy_file_range to copy across
> >> > > > > devices")
> >> > > > > and
> >> > > > > removes generic_copy_file_range() calls from ceph, cifs, fuse,
> >> > > > > and
> >> > > > > nfs.
> >> > > > >
> >> > > > > Fixes: 5dae222a5ff0 ("vfs: allow copy_file_range to copy across
> >> > > > > devices")
> >> > > > > Link:
> >> > > > > https://lore.kernel.org/linux-fsdevel/20210212044405.4120619-1-drink...@chromium.org/
> >> > > > > Cc: Nicolas Boichat
> >> > > > > Signed-off-by: Luis Henriques
> >> > > >
> >> > > > Code looks ok.
> >> > > > You may add:
> >> > > >
> >> > > > Reviewed-by: Amir Goldstein
> >> > > >
> >> > > > I agree with Trond that the first paragraph of the commit message
> >> > > > could
> >> > > > be improved.
> >> > > > The purpose of this change is to fix the change of behavior that
> >> > > > caused the regression.
> >> > > >
> >> > > > Before v5.3, behavior was -EXDEV and userspace could fallback to
> >> > > > read.
> >> > > > After v5.3, behavior is zero size copy.
> >> > > >
> >> > > > It does not matter so much what makes sense for CFR to do in this
> >> > > > case (generic cross-fs copy). What matters is that nobody asked
> >> > > > for
> >> > > > this change and that it caused problems.
> >> > > >
> >> > >
> >> > > No. I'm saying that this patch should be NACKed unless there is a
> >> > > real
> >> > > explanation for why we give crap about this tracefs corner case and
> >> > > why
> >> > > it can't be fixed.
> >> > >
> >> > > There are plenty of reasons why copy offload across filesystems
> >> > > makes
> >> > > sense, and particularly when you're doing NAS. Clone just doesn't
> >> > > cut
> >> > > it when it comes to disaster recovery (whereas backup to a
> >> > > different
> >> > > storage unit does). If the client has to do the copy, then you're
> >> > > effectively doubling the load on the server, and you're adding
> >> > > potentially unnecessary network traffic (or at the very least you
> >> > > are
> >> > > doubling that traffic).
> >> > >
> >> >
> >> > I don't understand the use case you are describing.
> >> >
> >> > Which filesystem types are you talking about for source and target
> >> > of copy_file_range()?
> >> >
> >> > To be clear, the original change was done to support NFS/CIFS server-
> >> > side
> >> > copy and those should not be affected by this change.
> >> >
> >>
> >> That is incorrect:
> >>
> >> ssize_t nfsd_copy_file_range(struct file *src, u64 src_pos, struct file
> >> *dst,
> >> u64 dst_pos, u64 count)
> >> {
> >>
> >> /*
> >> * Limit copy to 4MB to prevent indefinitely blocking an nfsd
> >> * thread and client rpc slot. The choice of 4MB is somewhat
> >> * arbitrary. We might instead base this on r/wsize, or make it
> >> * tunable, or use a time instead of a byte limit, or implement
> >> * asynchronous copy. In theory a client could also recognize a
> >> * limit like this and pipeline multiple COPY requests.
> >> */
> >> count = min_t(u64, count, 1 << 22);
> >> return vfs_copy_file_range(src, src_pos, dst, dst_pos, count, 0);
> >> }
> >>
> >> You are now explicitly changing the behaviour of knfsd when the source
> >> and destination filesystem differ.
> >>
> >> For one thing, you are disallowing the NFSv4.2 copy offload use case of
> >> copying from a local filesystem to a remote NFS server. However you are
> >> also disallowing the copy from, say, an XFS formatted partition to an
> >> ext4 partition.
> >>
> >
> > Got it.
>
> Ugh. And I guess overlayfs may have a similar problem.
>
> > This is easy to solve with a flag COPY_FILE_SPLICE (or something) that
> > is internal to kernel users.
> >
> > FWIW, you may want to look at the loop in ovl_copy_up_data()
> > for improvements to nfsd_copy_file_range().
> >
> > We can move the check out to copy_file_range syscall:
> >
> > if (flags != 0)
> > return -EINVAL;
> >
> > Leave the fallback from all filesystems and check for the
> >
Re: [RFC PATCH 1/7] drivers: base: Add resource managed version of delayed work init
On Mon, Feb 15, 2021 at 11:37:26AM +0100, Hans de Goede wrote:
> Hi,
>
> On 2/15/21 8:22 AM, Vaittinen, Matti wrote:
> >
> > On Sat, 2021-02-13 at 16:59 +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 2/13/21 4:27 PM, Guenter Roeck wrote:
> >>> On 2/13/21 7:03 AM, Hans de Goede wrote:
> >>> [ ... ]
> I think something like this should work:
>
> static int devm_delayed_work_autocancel(struct device *dev,
> struct delayed_work *w,
> void (*worker)(struct
> work_struct *work)) {
> INIT_DELAYED_WORK(w, worker);
> return devm_add_action(dev, (void (*action)(void
> *))cancel_delayed_work_sync, w);
> }
>
> I'm not sure about the cast, that may need something like this
> instead:
>
> typedef void (*devm_action_func)(void *);
>
> static int devm_delayed_work_autocancel(struct device *dev,
> struct delayed_work *w,
> void (*worker)(struct
> work_struct *work)) {
> INIT_DELAYED_WORK(w, worker);
> return devm_add_action(dev,
> (devm_action_func)cancel_delayed_work_sync, w);
> >>>
> >>> Unfortunately, you can not type cast function pointers in C. It is
> >>> against the C ABI.
> >>> I am sure it is done in a few places in the kernel anyway, but
> >>> those are wrong.
> >>
> >> I see, bummer.
> >
> > I think using devm_add_action() is still a good idea.
>
> Yes, we could also just have a 1 line static inline function to do
> the function-cast. Like this:
>
> static inline void devm_delayed_work_autocancel_func(void *work)
> {
> cancel_delayed_work_sync(work);
> }
>
> static inline int devm_delayed_work_autocancel(struct device *dev, struct
> delayed_work *w, void (*worker)(struct work_struct *work))
> {
> INIT_DELAYED_WORK(w, worker);
> return devm_add_action(dev, devm_delayed_work_autocancel_func, w);
> }
>
> Both functions will then simply be compiled out in files which do not
> use them.
>
> >> If we add a devm_clk_prepare_enable() helper that should probably be
> >> added
> >> to drivers/clk/clk-devres.c and not to drivers/base/devres.c .
> >>
> >> I also still wonder if we cannot find a better place for this new
> >> devm_delayed_work_autocancel() helper but nothing comes to mind.
> >
> > I don't like the idea of including device.h from workqueue.h - and I
> > think this would be necessary if we added
> > devm_delayed_work_autocancel() as inline in workqueue.h, right?
>
> Yes.
>
> > I also see strong objection towards the devm managed clean-ups.
>
> Yes it seems that there are some people who don't like this, where as
> others do like them.
>
> > How about adding some devm-helpers.c in drivers/base - where we could
> > collect devm-based helpers - and which could be enabled by own CONFIG -
> > and left out by those who dislike it?
>
> I would make this something configurable through Kconfig, but if
> go the static inline route, which I'm in favor of then we could just
> have a:
>
> include/linux/devm-cleanup-helpers.h
>
> And put everything (including kdoc texts) there.
>
> This way the functionality is 100% opt-in (by explicitly including
> the header if you want the helpers) which hopefully makes this a
> bit more acceptable to people who don't like this style of cleanups.
>
> I would be even happy to act as the upstream maintainer for such a
> include/linux/devm-cleanup-helpers.h file, I can maintain it as part
> of the platform-drivers-x86 tree (with its own MAINTAINERS entry).
>
> Greg, would this be an acceptable solution to you ?
I don't know, sorry, let's revisit this after 5.12-rc1 is out, with a
patch set that I can review again, and we can go from there as I can't
do anything until then...
thanks,
greg k-h
Re: [RFC PATCH 1/7] drivers: base: Add resource managed version of delayed work init
On Sat, Feb 13, 2021 at 12:26:59PM +, Vaittinen, Matti wrote:
>
> On Sat, 2021-02-13 at 13:16 +0100, Greg Kroah-Hartman wrote:
> > On Sat, Feb 13, 2021 at 01:58:44PM +0200, Matti Vaittinen wrote:
> > > A few drivers which need a delayed work-queue must cancel work at
> > > exit.
> > > Some of those implement remove solely for this purpose. Help
> > > drivers
> > > to avoid unnecessary remove and error-branch implementation by
> > > adding
> > > managed verision of delayed work initialization
> > >
> > > Signed-off-by: Matti Vaittinen
> >
> > That's not a good idea. As this would kick in when the device is
> > removed from the system, not when it is unbound from the driver,
> > right?
> >
> > > ---
> > > drivers/base/devres.c | 33 +
> > > include/linux/device.h | 5 +
> > > 2 files changed, 38 insertions(+)
> > >
> > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> > > index fb9d5289a620..2879595bb5a4 100644
> > > --- a/drivers/base/devres.c
> > > +++ b/drivers/base/devres.c
> > > @@ -1231,3 +1231,36 @@ void devm_free_percpu(struct device *dev,
> > > void __percpu *pdata)
> > > (void *)pdata));
> > > }
> > > EXPORT_SYMBOL_GPL(devm_free_percpu);
> > > +
> > > +static void dev_delayed_work_drop(struct device *dev, void *res)
> > > +{
> > > + cancel_delayed_work_sync(*(struct delayed_work **)res);
> > > +}
> > > +
> > > +/**
> > > + * devm_delayed_work_autocancel - Resource-managed work allocation
> > > + * @dev: Device which lifetime work is bound to
> > > + * @pdata: work to be cancelled when device exits
> > > + *
> > > + * Initialize work which is automatically cancelled when device
> > > exits.
> >
> > There is no such thing in the driver model as "when device exits".
> > Please use the proper terminology as I do not understand what you
> > think
> > this is doing here...
> >
> > > + * A few drivers need delayed work which must be cancelled before
> > > driver
> > > + * is unload to avoid accessing removed resources.
> > > + * devm_delayed_work_autocancel() can be used to omit the explicit
> > > + * cancelleation when driver is unload.
> > > + */
> > > +int devm_delayed_work_autocancel(struct device *dev, struct
> > > delayed_work *w,
> > > + void (*worker)(struct work_struct
> > > *work))
> > > +{
> > > + struct delayed_work **ptr;
> > > +
> > > + ptr = devres_alloc(dev_delayed_work_drop, sizeof(*ptr),
> > > GFP_KERNEL);
> > > + if (!ptr)
> > > + return -ENOMEM;
> > > +
> > > + INIT_DELAYED_WORK(w, worker);
> > > + *ptr = w;
> > > + devres_add(dev, ptr);
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(devm_delayed_work_autocancel);
> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index 1779f90eeb4c..192456198de7 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -27,6 +27,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -249,6 +250,10 @@ void __iomem *devm_of_iomap(struct device
> > > *dev,
> > > struct device_node *node, int index,
> > > resource_size_t *size);
> > >
> > > +/* delayed work which is cancelled when driver exits */
> >
> > Not when the "driver exits".
> >
> > There is two different lifespans here (well 3). Code and
> > data*2. Don't
> > confuse them as that will just cause lots of problems.
> >
> > The move toward more and more "devm" functions is not the way to go
> > as
> > they just more and more make things easier to get wrong.
> >
> > APIs should be impossible to get wrong, this one is going to be
> > almost
> > impossible to get right.
>
> Thanks for prompt reply. I guess I must've misunderstood some of this
> concept. Frankly to say, I don't understand how the devm based irq
> management works and this does not. Maybe I'd better study this further
> then.
The devm based irq management works horribly and should not be used at
all. That is the main offender in the "an api that is impossible to use
correctly" category.
Honestly, I think it should just be removed as there is almost no real
need for it that I can determine, other than to cause bugs.
thanks,
greg k-h
Re: [PATCH v4] staging: gdm724x: Fix DMA from stack
On Thu, Feb 11, 2021 at 01:28:41PM +, David Laight wrote: > > Stack allocated buffers cannot be used for DMA > > on all architectures so allocate hci_packet buffer > > using kmalloc. > > I wonder if the usb stack ought/could support a short bounce > buffer within the memory is already has to allocate? No.
Re: [PATCH 2/3] kbuild: clamp SUBLEVEL to 255
On Mon, Feb 08, 2021 at 01:48:06PM +, David Laight wrote: > From: Sasha Levin > > Sent: 06 February 2021 03:51 > > > > Right now if SUBLEVEL becomes larger than 255 it will overflow into the > > territory of PATCHLEVEL, causing havoc in userspace that tests for > > specific kernel version. > > > > While userspace code tests for MAJOR and PATCHLEVEL, it doesn't test > > SUBLEVEL at any point as ABI changes don't happen in the context of > > stable tree. > > > > Thus, to avoid overflows, simply clamp SUBLEVEL to it's maximum value in > > the context of LINUX_VERSION_CODE. This does not affect "make > > kernelversion" and such. > > > > Signed-off-by: Sasha Levin > > --- > > Makefile | 12 +--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index 49ac1b7fe8e99..157be50c691e5 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -1258,9 +1258,15 @@ define filechk_utsrelease.h > > endef > > > > define filechk_version.h > > - echo \#define LINUX_VERSION_CODE $(shell \ > > - expr $(VERSION) \* 65536 + 0$(PATCHLEVEL) \* 256 + 0$(SUBLEVEL)); \ > > - echo '#define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))' > > + if [ $(SUBLEVEL) -gt 255 ]; then \ > > + echo \#define LINUX_VERSION_CODE $(shell \ > > + expr $(VERSION) \* 65536 + 0$(PATCHLEVEL) \* 256 + 255); \ > > + else \ > > + echo \#define LINUX_VERSION_CODE $(shell \ > > + expr $(VERSION) \* 65536 + 0$(PATCHLEVEL) \* 256 + > > $(SUBLEVEL)); \ > > + fi; \ > > + echo '#define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + \ > > + ((c) > 255 ? 255 : (c)))' > > endef > > Why not use KERNEL_VERSION to define LINUX_VERSION_CODE ? > Basically just: > echo '#define LINUX_VERSION_CODE KERNEL_VERSION($(VERSION), > $(PATCHLEVEL)+0, $(SUBLEVEL)+0)' Because we are "clamping" LINUX_VERSION_CODE() at a x.y.255, while KERNEL_VERSION() continues on with the "real" minor number. thanks, greg k-h
Re: [PATCH 3/8] scsi: ufshpb: Add region's reads counter
On Mon, Feb 01, 2021 at 08:17:59AM +, Avri Altman wrote: > > > > On Mon, Feb 01, 2021 at 07:51:19AM +, Avri Altman wrote: > > > > > > > > On Mon, Feb 01, 2021 at 07:12:53AM +, Avri Altman wrote: > > > > > > > +#define WORK_PENDING 0 > > > > > > > +#define ACTIVATION_THRSHLD 4 /* 4 IOs */ > > > > > > Rather than fixing it with macro, how about using sysfs and make it > > > > > > configurable? > > > > > Yes. > > > > > I will add a patch making all the logic configurable. > > > > > As all those are hpb-related parameters, I think module parameters are > > > > more adequate. > > > > > > > > No, this is not the 1990's, please never add new module parameters to > > > > drivers. If not for the basic problem of they do not work on a > > > > per-device basis, but on a per-driver basis, which is what you almost > > > > never want. > > > OK. > > > > > > > > > > > But why would you want to change this value, why can't the driver "just > > > > work" and not need manual intervention? > > > It is. > > > But those are a knobs each vendor may want to tweak, > > > So it'll be optimized with its internal device's implementation. > > > > > > Tweaking the parameters, as well as the entire logic, is really an endless > > task. > > > Some logic works better for some scenarios, while falling behind on > > > others. > > > > Shouldn't the hardware know how to handle this dynamically? If not, how > > is a user going to know? > There is one "brain". > It is either in the device - in device mode, Or in the host - in host mode > control. > The "brain" decides which region is active, thus carrying the physical > address along with the logical - > minimizing context switches in the device's RAM. > > There can be up to N active regions. > Activation and deactivation has its overhead. > So basically it is a constraint-optimization problem. So how do you solve it? And how would you expect a user to solve it if the kernel can not? You better document the heck out of these configuration options :) thanks, greg k-h
Re: [PATCH 3/8] scsi: ufshpb: Add region's reads counter
On Mon, Feb 01, 2021 at 07:51:19AM +, Avri Altman wrote: > > > > On Mon, Feb 01, 2021 at 07:12:53AM +, Avri Altman wrote: > > > > > +#define WORK_PENDING 0 > > > > > +#define ACTIVATION_THRSHLD 4 /* 4 IOs */ > > > > Rather than fixing it with macro, how about using sysfs and make it > > > > configurable? > > > Yes. > > > I will add a patch making all the logic configurable. > > > As all those are hpb-related parameters, I think module parameters are > > more adequate. > > > > No, this is not the 1990's, please never add new module parameters to > > drivers. If not for the basic problem of they do not work on a > > per-device basis, but on a per-driver basis, which is what you almost > > never want. > OK. > > > > > But why would you want to change this value, why can't the driver "just > > work" and not need manual intervention? > It is. > But those are a knobs each vendor may want to tweak, > So it'll be optimized with its internal device's implementation. > > Tweaking the parameters, as well as the entire logic, is really an endless > task. > Some logic works better for some scenarios, while falling behind on others. Shouldn't the hardware know how to handle this dynamically? If not, how is a user going to know? > How about leaving it for now, to be elaborated it in the future? I do not care, just do not make it a module parameter for the reason that does not work on a per-device basis. > Maybe even can be a part of a scheme, to make the logic proprietary? What do you mean by "proprietary"? thanks, greg k-h
Re: [PATCH 3/8] scsi: ufshpb: Add region's reads counter
On Mon, Feb 01, 2021 at 07:12:53AM +, Avri Altman wrote: > > > +#define WORK_PENDING 0 > > > +#define ACTIVATION_THRSHLD 4 /* 4 IOs */ > > Rather than fixing it with macro, how about using sysfs and make it > > configurable? > Yes. > I will add a patch making all the logic configurable. > As all those are hpb-related parameters, I think module parameters are more > adequate. No, this is not the 1990's, please never add new module parameters to drivers. If not for the basic problem of they do not work on a per-device basis, but on a per-driver basis, which is what you almost never want. But why would you want to change this value, why can't the driver "just work" and not need manual intervention? thanks, greg k-h
Re: [PATCH v3 00/12] add IRQF_NO_AUTOEN for request_irq
On Thu, Jan 21, 2021 at 09:38:28PM +, Song Bao Hua (Barry Song) wrote: > Hi Thomas, Greg, Dmitry, Marc, > Any further comment on this new API? It's not my subsystem, I'll let the irq maintainers handle it :) thanks, greg k-h
Re: [PATCH] rtsx: pci: fix device aspm state bug
On Thu, Jan 21, 2021 at 08:15:46AM +, 吳昊澄 Ricky wrote: > > -Original Message- > > From: gre...@linuxfoundation.org > > Sent: Thursday, January 21, 2021 4:07 PM > > To: 吳昊澄 Ricky > > Cc: a...@arndb.de; ricky...@realtek.corp-partner.google.com; > > sas...@kernel.org; levin...@google.com; > > keitasuzuki.p...@sslab.ics.keio.ac.jp; > > kd...@doth.eu; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH] rtsx: pci: fix device aspm state bug > > > > On Thu, Jan 21, 2021 at 07:33:03AM +, 吳昊澄 Ricky wrote: > > > Hi Greg kh, > > > > > > This patch to fix misc: rtsx bug for kernel 5.4 > > > > I do not understand what this means, sorry. Can you please explain it? > > > On the newest upstream we don’t set config space for en/disable aspm, > so it will not happen this issue > but on kernel 5.4 longterm version we need to fix it Please read https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to submit patches to the stable tree properly. And if this is not an issue in Linus's tree, why not just backport the commits that fixed this issue? That's the best way to do this. thanks, greg k-h
Re: [PATCH] rtsx: pci: fix device aspm state bug
On Thu, Jan 21, 2021 at 07:33:03AM +, 吳昊澄 Ricky wrote: > Hi Greg kh, > > This patch to fix misc: rtsx bug for kernel 5.4 I do not understand what this means, sorry. Can you please explain it? greg k-h
Re: [PATCH v3] USB: serial: cp210x: Fix error 32 when hardware flow control is enabled.
On Thu, Jan 21, 2021 at 07:09:13AM +, Pho Tran wrote:
> Fix error 32 returned by CP210X_SET_MHS when hardware flow control is enabled.
>
> The root cause of error 32 is that user application (CoolTerm,
> linux-serial-test)
> opened cp210x device with hardware flow control then attempt to control
> RTS/DTR pins.
> In hardware flow control, RTS/DTR pins will be controlled by hardware only,
> any attempt to control those pins will cause error 32 from the device.
> This fix will block MHS command(command to control RTS/DTR pins) to the device
> if hardware flow control is being used.
>
> Signed-off-by: Pho Tran
> ---
> 01/19/2021: Patch v2 Modified based on comment from Johan Hovold
>
> and Greg Kroah-Hartman
> ---
> drivers/usb/serial/cp210x.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index fbb10dfc56e3..5b6bbda2b424 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -1204,7 +1204,12 @@ static int cp210x_tiocmset(struct tty_struct *tty,
> unsigned int set, unsigned int clear)
> {
> struct usb_serial_port *port = tty->driver_data;
> - return cp210x_tiocmset_port(port, set, clear);
> + if (C_CRTSCTS(tty))
> +
> + /* Don't send SET_MHS command if device in hardware flow control mode.
> */
> + return 0;
That indentation and whitespace is very odd. Did you run checkpatch.pl
on your change before sending it to us to verify that all is ok?
Please do so.
thanks,
greg k-h
Re: [PATCH] USB: serial: cp210x: Fix error 32 when hardware flow control is enabled.
On Tue, Jan 19, 2021 at 09:43:13AM +, Pho Tran wrote:
> Fix error 32 returned by CP210X_SET_MHS when hardware flow control is enabled.
>
> The root cause of error 32 is that user application (CoolTerm,
> linux-serial-test)
> opened cp210x device with hardware flow control then attempt to control
> RTS/DTR pins.
> In hardware flow control, RTS/DTR pins will be controlled by hardware only,
> any attempt to control those pin will cause error 32 from the device.
> This fix will block MHS command(command to control RTS/DTR pins) to the device
> if hardware flow control is being used.
>
> Signed-off-by: Pho Tran
> ---
> drivers/usb/serial/cp210x.c | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index fbb10dfc56e3..1835ccf2aa2f 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -1211,6 +1211,11 @@ static int cp210x_tiocmset_port(struct usb_serial_port
> *port,
> unsigned int set, unsigned int clear)
> {
> u16 control = 0;
> + struct cp210x_flow_ctl flow_ctl;
> + u32 ctl_hs = 0;
> + u32 flow_repl = 0;
> + bool auto_dtr = false;
> + bool auto_rts = false;
>
> if (set & TIOCM_RTS) {
> control |= CONTROL_RTS;
> @@ -1230,6 +1235,25 @@ static int cp210x_tiocmset_port(struct usb_serial_port
> *port,
> }
>
> dev_dbg(>dev, "%s - control = 0x%.4x\n", __func__, control);
> + //Don't send MHS command if device in hardware flowcontrol mode
Please put a blank line before this comment.
And a ' ' after "//".
> + cp210x_read_reg_block(port, CP210X_GET_FLOW, _ctl,
> sizeof(flow_ctl));
No error checking?
> + ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake);
> + flow_repl = le32_to_cpu(flow_ctl.ulFlowReplace);
> +
> + if (CP210X_SERIAL_DTR_SHIFT(CP210X_SERIAL_DTR_FLOW_CTL) == (ctl_hs &
> CP210X_SERIAL_DTR_MASK))
> + auto_dtr = true;
> + else
> + auto_dtr = false;
> +
> + if (CP210X_SERIAL_RTS_SHIFT(CP210X_SERIAL_RTS_FLOW_CTL) == (flow_repl &
> CP210X_SERIAL_RTS_MASK))
> + auto_rts = true;
> + else
> + auto_rts = false;
> +
> + if (auto_dtr || auto_rts) {
> + dev_dbg(>dev, "Don't set MHS when while device in flow
> control mode\n");
> + return 0;
Shouldn't this be a real error to send back to userspace, so that they
know the change they asked for failed?
thanks,
greg k-h
Re: [PATCH v3 1/1] kdump: append uts_namespace.name offset to VMCOREINFO
On Fri, Jan 08, 2021 at 06:22:24PM +0800, Baoquan He wrote:
> On 01/08/21 at 10:07am, HAGIO KAZUHITO(萩尾 一仁) wrote:
> > Hi Baoquan,
> >
> > -Original Message-
> > > On 09/30/20 at 12:23pm, Alexander Egorenkov wrote:
> > > > The offset of the field 'init_uts_ns.name' has changed
> > > > since commit 9a56493f6942 ("uts: Use generic ns_common::count").
> > >
> > > This patch is merged into 5.11-rc1, but we met the makedumpfile failure
> > > of kdump test case in 5.10.0 kernel. Should affect 5.9 too since
> > > commit 9a56493f6942 is merged into 5.9-rc2.
> >
> > Hmm, commit 9a56493f6942 should have been merged into 5.11-rc1
> > together with commit ca4a9241cc5e.
>
> Checked on master branch of mainline kernel, commit 9a56493f6942 is in
> 5.9-rc1.
No, that commit is in 5.11-rc1, not 5.9-rc1:
$ git describe --contains 9a56493f6942
v5.11-rc1~182^2~9
> commit ca4a9241cc5e is merged into 5.11-rc1.
>
> commit 9a56493f6942c0e2df1579986128721da96e00d8
> Author: Kirill Tkhai
> Date: Mon Aug 3 13:16:21 2020 +0300
>
> uts: Use generic ns_common::count
>
>
> commit ca4a9241cc5e718de86a34afd41972869546a5e3
> Author: Alexander Egorenkov
> Date: Tue Dec 15 20:45:31 2020 -0800
>
> kdump: append uts_namespace.name offset to VMCOREINFO
Are you all sure this is needed in 5.10.y?
thanks,
greg k-h
Re: [PATCH] USB: otg: Fix error 32 when enable hardware flow control.
On Mon, Jan 11, 2021 at 04:55:22AM +, Pho Tran wrote:
> When hardware flow control is enabled,
> don't allow host send MHS command to cp210x.
>
> Signed-off-by: Pho Tran
Nit, you need a ' ' before the '<' character.
And why didn't you send this to the maintainer of this file as described
by scripts/get_maintainer.pl?
Please do so when you fix things up and send the next version.
> ---
> drivers/usb/serial/cp210x.c | 32 +++-
> 1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index fbb10dfc56e3..f231cecdaf7d 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -1204,6 +1204,7 @@ static int cp210x_tiocmset(struct tty_struct *tty,
> unsigned int set, unsigned int clear)
> {
> struct usb_serial_port *port = tty->driver_data;
> +
> return cp210x_tiocmset_port(port, set, clear);
> }
>
Unneeded change :(
> @@ -1211,6 +1212,11 @@ static int cp210x_tiocmset_port(struct usb_serial_port
> *port,
> unsigned int set, unsigned int clear)
> {
> u16 control = 0;
> + struct cp210x_flow_ctl flow_ctl;
> + u32 ctl_hs = 0;
> + u32 flow_repl = 0;
> + bool auto_dtr = false;
> + bool auto_rts = false;
>
> if (set & TIOCM_RTS) {
> control |= CONTROL_RTS;
> @@ -1230,6 +1236,30 @@ static int cp210x_tiocmset_port(struct usb_serial_port
> *port,
> }
>
> dev_dbg(>dev, "%s - control = 0x%.4x\n", __func__, control);
> + /*
> + * Don't send MHS command if device in hardware flowcontrol mode
> + */
Why the giant comment?
> + cp210x_read_reg_block(port, CP210X_GET_FLOW, _ctl,
> + sizeof(flow_ctl));
> + ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake);
> + flow_repl = le32_to_cpu(flow_ctl.ulFlowReplace);
> +
> + if (CP210X_SERIAL_DTR_SHIFT(CP210X_SERIAL_DTR_FLOW_CTL)
> + == (ctl_hs & CP210X_SERIAL_DTR_MASK))
Very odd indentation :(
> + auto_dtr = true;
> + else
> + auto_dtr = false;
> +
> + if (CP210X_SERIAL_RTS_SHIFT(CP210X_SERIAL_RTS_FLOW_CTL)
> + == (flow_repl & CP210X_SERIAL_RTS_MASK))
> + auto_rts = true;
> + else
> + auto_rts = false;
> +
> + if (auto_dtr || auto_rts) {
> + dev_dbg(>dev, "Don't set MHS when while device in flow
> control mode\n");
> + return 0;
> + }
>
> return cp210x_write_u16_reg(port, CP210X_SET_MHS, control);
> }
> @@ -1256,7 +1286,7 @@ static int cp210x_tiocmget(struct tty_struct *tty)
> |((control & CONTROL_RTS) ? TIOCM_RTS : 0)
> |((control & CONTROL_CTS) ? TIOCM_CTS : 0)
> |((control & CONTROL_DSR) ? TIOCM_DSR : 0)
> - |((control & CONTROL_RING)? TIOCM_RI : 0)
> + |((control & CONTROL_RING) ? TIOCM_RI : 0)
Do not mix whitespace changes with logic changes in the same patch, that
is a sure way to get your patch rejected.
thanks,
greg k-h
Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
On Fri, Jan 08, 2021 at 02:19:30AM +, Thinh Nguyen wrote:
> Hi Wesley,
>
> Felipe Balbi wrote:
> > Hi,
> >
> > Wesley Cheng writes:
> >> +void composite_reset(struct usb_gadget *gadget)
> >> +{
> >> + /*
> >> + * Section 1.4.13 Standard Downstream Port of the USB battery charging
> >> + * specification v1.2 states that a device connected on a SDP shall only
> >> + * draw at max 100mA while in a connected, but unconfigured state.
> > The requirements are different, though. I think OTG spec has some extra
> > requirements where only 8mA can be drawn max. You need to check for the
> > otg flag. Moreover, USB3+ spec has units of 150mA meaning the device
> > can't draw 100mA (IIRC).
> >
>
> We see issue with this patch series. For our device running at SSP, the
> device couldn't recover from a port reset and remained in eSS.Inactive
> state.
>
> This patch series is already in Greg's usb-testing. Please review and
> help fix it.
Should I just revert this? I'll be glad to drop it.
thanks,
greg k-h
Re: [PATCH] uio: uio_pci_generic: don't fail probe if pdev->irq equals to IRQ_NOTCONNECTED
On Thu, Jan 07, 2021 at 07:32:07PM +0800, 李捷 wrote: > >From 0fbcd7e386898d829d3000d094358a91e626ee4a Mon Sep 17 00:00:00 2001 > From: Jie Li > Date: Mon, 7 Dec 2020 08:05:07 +0800 > Subject: [PATCH] uio: uio_pci_generic: don't fail probe if pdev->irq equals to > IRQ_NOTCONNECTED Why is this in the body of the email? Please just use 'git send-email' or some other sane email client to send patches out so that we can apply them correctly. thanks, greg k-h
Re: [PATCH] uio: uio_pci_generic: don't fail probe if pdev->irq equals to IRQ_NOTCONNECTED
On Thu, Jan 07, 2021 at 03:50:37PM +0800, 李捷 wrote:
> From 0fbcd7e386898d829d3000d094358a91e626ee4a Mon Sep 17 00:00:00 2001
> From: Jie Li
> Date: Mon, 7 Dec 2020 08:05:07 +0800
> Subject: [PATCH] uio: uio_pci_generic: don't fail probe if pdev->irq equals to
> IRQ_NOTCONNECTED
>
> Some devices use 255 as default value of Interrupt Line register, and this
> maybe causes pdev->irq is set as IRQ_NOTCONNECTED in some scenarios. For
> example, NVMe controller connects to Intel Volume Management Device (VMD).
> In this situation, IRQ_NOTCONNECTED means INTx line is not connected, not
> fault. If bind uio_pci_generic to these devices, uio frame will return
> -ENOTCONN through request_irq.
>
> This patch allows binding uio_pci_generic to device with dev->irq of
> IRQ_NOTCONNECTED.
>
> Signed-off-by: Jie Li
> Acked-by: Kyungsan Kim
> ---
> drivers/uio/uio_pci_generic.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
> index b8e44d16279f..c7d681fef198 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -92,7 +92,7 @@ static int probe(struct pci_dev *pdev,
> gdev->info.version = DRIVER_VERSION;
> gdev->info.release = release;
> gdev->pdev = pdev;
> - if (pdev->irq) {
> + if (pdev->irq && (pdev->irq != IRQ_NOTCONNECTED)) {
> gdev->info.irq = pdev->irq;
> gdev->info.irq_flags = IRQF_SHARED;
> gdev->info.handler = irqhandler;
> --
> 2.17.1
>
>
>
>
> [cid]
>
> *
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- Your patch was attached, please place it inline so that it can be
applied directly from the email message itself.
- Your email was sent in html format, which is rejected by the kernel
mailing lists and make it impossible for anyone to find in the
archives.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
Re: [PATCH] uio: uio_pci_generic: don't fail probe if pdev->irq equals to IRQ_NOTCONNECTED
On Wed, Jan 06, 2021 at 07:48:51PM +0800, 李捷 wrote:
> From 0fbcd7e386898d829d3000d094358a91e626ee4a Mon Sep 17 00:00:00 2001
> From: Jie Li
> Date: Mon, 7 Dec 2020 08:05:07 +0800
> Subject: [PATCH] uio: uio_pci_generic: don't fail probe if pdev->irq equals to
> IRQ_NOTCONNECTED
>
> Some devices use 255 as default value of Interrupt Line register, and this
> maybe causes pdev->irq is set as IRQ_NOTCONNECTED in some scenarios. For
> example, NVMe controller connects to Intel Volume Management Device (VMD).
> In this situation, IRQ_NOTCONNECTED means INTx line is not connected, not
> fault. If bind uio_pci_generic to these devices, uio frame will return
> -ENOTCONN through request_irq.
>
> This patch allows binding uio_pci_generic to device with dev->irq of
> IRQ_NOTCONNECTED.
>
> Signed-off-by: Jie Li
> Acked-by: Kyungsan Kim
> ---
> drivers/uio/uio_pci_generic.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
> index b8e44d16279f..c7d681fef198 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -92,7 +92,7 @@ static int probe(struct pci_dev *pdev,
> gdev->info.version = DRIVER_VERSION;
> gdev->info.release = release;
> gdev->pdev = pdev;
> - if (pdev->irq) {
> + if (pdev->irq && (pdev->irq != IRQ_NOTCONNECTED)) {
>gdev->info.irq = pdev->irq;
>gdev->info.irq_flags = IRQF_SHARED;
>gdev->info.handler = irqhandler;
> --
> 2.17.1
>
>
>
>
>
> Best regards,
>
> Jie Li
>
> [cid]
>
> *
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- Your patch is malformed (tabs converted to spaces, linewrapped, etc.)
and can not be applied. Please read the file,
Documentation/email-clients.txt in order to fix this.
- Your patch was attached, please place it inline so that it can be
applied directly from the email message itself.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
Re: [PATCH v2 09/12] media: atomisp: Fix PARENTHESIS_ALIGNMENT
On Mon, Dec 14, 2020 at 11:49:56AM +, David Laight wrote: > From: Philipp Gerlesberger > > Sent: 14 December 2020 11:04 > > > > You can sum up the two lines, because the maximum line length of > > 100 columns is not exceeded. > > IIRC the 80 column limit is preferred. Not anymore, checkpatch has changed.
Re: [PATCH] usb: cdns3: Fixes for sparse warnings
On Mon, Dec 14, 2020 at 11:26:22AM +, David Laight wrote: > From: Pawel Laszczak > > Sent: 14 December 2020 11:05 > > > > Patch fixes the following warnings: > ... > > cdns3-ep0.c:367: sparse: warning: restricted __le16 degrades to integer > ... > > diff --git a/drivers/usb/cdns3/cdns3-ep0.c b/drivers/usb/cdns3/cdns3-ep0.c > > index b0390fe9a396..9a17802275d5 100644 > > --- a/drivers/usb/cdns3/cdns3-ep0.c > > +++ b/drivers/usb/cdns3/cdns3-ep0.c > > @@ -364,7 +364,7 @@ static int cdns3_ep0_feature_handle_endpoint(struct > > cdns3_device *priv_dev, > > if (le16_to_cpu(ctrl->wValue) != USB_ENDPOINT_HALT) > > return -EINVAL; > > > > - if (!(ctrl->wIndex & ~USB_DIR_IN)) > > + if (!(le16_to_cpu(ctrl->wIndex) & ~USB_DIR_IN)) > > return 0; > > It's generally best to byteswap the constant. Why? This is fine, it's better to operate on the value that needs to be operated on. greg k-h
Re: [PATCH 16/22] xlink-ipc: Add xlink ipc driver
On Fri, Dec 11, 2020 at 11:33:02AM +, Kelly, Seamus wrote: > This e-mail and any attachments may contain confidential material for the sole > use of the intended recipient(s). Any review or distribution by others is > strictly prohibited. If you are not the intended recipient, please contact the > sender and delete all copies. Now deleted! This footer is incompatible with Linux kernel development, please remove it in order for us to read your emails and do anything with them. I can't believe that Intel still has this problem, after all of these years... greg k-h
Re: [PATCH v2 03/10] usb: cdns3: Moves reusable code to separate module
On Tue, Nov 10, 2020 at 11:21:22AM +, Peter Chen wrote: > On 20-11-10 09:20:54, Pawel Laszczak wrote: > > Hi, > > > > >> > > >> int cdns3_hw_role_switch(struct cdns3 *cdns); > > >> -int cdns3_init(struct cdns3 *cdns); > > >> -int cdns3_remove(struct cdns3 *cdns); > > >> +extern int cdns3_init(struct cdns3 *cdns); > > >> +extern int cdns3_remove(struct cdns3 *cdns); > > > > > >Why add "extern" here and below? > > > > > > > These functions are the API between cdnsp and cdns3 modules. > > It's looks like a common approach in kernel. > > Many or even most of API function in kernel has "extern". > > > > Even you have not written "extern" keyword, the "extern" is > added implicitly by compiler. Usually, we use "extern" for variable > or the function is defined at assembly. You could see some > "extern" keyword use cases at include/linux/device.h. We are moving away from using this keyword for functions now, if at all possible please. Only use it for variables, I think checkpatch now catches it as well. thanks, greg k-h
Re: Question: gadget: How to realize uvc and uac composite function?
On Mon, Nov 09, 2020 at 02:03:11AM +, Tim Li wrote: > This email and any attachments thereto may contain private, confidential, and > privileged material for the sole use of the intended recipient. Any review, > copying, or distribution of this email (or any attachments thereto) by others > is strictly prohibited. If you are not the intended recipient, please contact > the sender immediately and permanently delete the original and any copies of > this email and any attachments thereto. Now deleted.
Re: Question: gadget: How to realize uvc and uac composite function?
On Fri, Nov 06, 2020 at 04:33:13AM +, Tim Li wrote: > This email and any attachments thereto may contain private, confidential, and > privileged material for the sole use of the intended recipient. Any review, > copying, or distribution of this email (or any attachments thereto) by others > is strictly prohibited. If you are not the intended recipient, please contact > the sender immediately and permanently delete the original and any copies of > this email and any attachments thereto. This footer is not compatible with posting on a public Linux mailing list, sorry. Now deleted...
Re: [PATCH 1/2] misc: c2port: core: Make copying name from userspace more secure
On Mon, Nov 02, 2020 at 12:43:01PM +, Lee Jones wrote: > On Mon, 02 Nov 2020, gre...@linuxfoundation.org wrote: > > > On Mon, Nov 02, 2020 at 11:49:03AM +, Lee Jones wrote: > > > On Mon, 02 Nov 2020, David Laight wrote: > > > > > > > From: Lee Jones > > > > > Sent: 02 November 2020 11:12 > > > > > > > > > > strncpy() may not provide a NUL terminator, which means that a 1-byte > > > > > leak would be possible *if* this was ever copied to userspace. Ensure > > > > > the buffer will always be NUL terminated by using the kernel's > > > > > strscpy() which a) uses the destination (instead of the source) size > > > > > as the bytes to copy and b) is *always* NUL terminated. > > > > > > > > > > Cc: Rodolfo Giometti > > > > > Cc: "Eurotech S.p.A" > > > > > Reported-by: Geert Uytterhoeven > > > > > Acked-by: Arnd Bergmann > > > > > Signed-off-by: Lee Jones > > > > > --- > > > > > drivers/misc/c2port/core.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/misc/c2port/core.c b/drivers/misc/c2port/core.c > > > > > index 80d87e8a0bea9..b96444ec94c7e 100644 > > > > > --- a/drivers/misc/c2port/core.c > > > > > +++ b/drivers/misc/c2port/core.c > > > > > @@ -923,7 +923,7 @@ struct c2port_device *c2port_device_register(char > > > > > *name, > > > > > } > > > > > dev_set_drvdata(c2dev->dev, c2dev); > > > > > > > > > > - strncpy(c2dev->name, name, C2PORT_NAME_LEN - 1); > > > > > + strscpy(c2dev->name, name, sizeof(c2dev->name)); > > > > > > > > strscpy() doesn't zero fill so if the memory isn't zeroed > > > > and a 'blind' copy to user of the structure is done > > > > then more data is leaked. > > > > > > > > strscpy() may be better, but rational isn't right. > > > > > > The original patch zeroed the data too, but I was asked to remove that > > > part [0]. In your opinion, should it be reinstated? > > > > > > [0] https://lore.kernel.org/patchwork/patch/1272290/ > > > > Just keep the kzalloc() part of the patch, this portion makes no sense > > to me. > > Can do. > > > But if you REALLY want to get it correct, call dev_set_name() > > instead please, as that is what it is there for. > > The line above isn't setting the 'struct device' name. It looks as > though 'struct c2port' has it's own member, also called 'name'. As to > how they differ, I'm not currently aware. Nor do I wish to mess > around with the semantics all that much. > > Going with suggestion #1. As the "device" already has a name, I suggest just getting rid of this name field anyway, no need for duplicates. thanks, greg k-h
Re: [PATCH 1/2] misc: c2port: core: Make copying name from userspace more secure
On Mon, Nov 02, 2020 at 11:49:03AM +, Lee Jones wrote: > On Mon, 02 Nov 2020, David Laight wrote: > > > From: Lee Jones > > > Sent: 02 November 2020 11:12 > > > > > > strncpy() may not provide a NUL terminator, which means that a 1-byte > > > leak would be possible *if* this was ever copied to userspace. Ensure > > > the buffer will always be NUL terminated by using the kernel's > > > strscpy() which a) uses the destination (instead of the source) size > > > as the bytes to copy and b) is *always* NUL terminated. > > > > > > Cc: Rodolfo Giometti > > > Cc: "Eurotech S.p.A" > > > Reported-by: Geert Uytterhoeven > > > Acked-by: Arnd Bergmann > > > Signed-off-by: Lee Jones > > > --- > > > drivers/misc/c2port/core.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/misc/c2port/core.c b/drivers/misc/c2port/core.c > > > index 80d87e8a0bea9..b96444ec94c7e 100644 > > > --- a/drivers/misc/c2port/core.c > > > +++ b/drivers/misc/c2port/core.c > > > @@ -923,7 +923,7 @@ struct c2port_device *c2port_device_register(char > > > *name, > > > } > > > dev_set_drvdata(c2dev->dev, c2dev); > > > > > > - strncpy(c2dev->name, name, C2PORT_NAME_LEN - 1); > > > + strscpy(c2dev->name, name, sizeof(c2dev->name)); > > > > strscpy() doesn't zero fill so if the memory isn't zeroed > > and a 'blind' copy to user of the structure is done > > then more data is leaked. > > > > strscpy() may be better, but rational isn't right. > > The original patch zeroed the data too, but I was asked to remove that > part [0]. In your opinion, should it be reinstated? > > [0] https://lore.kernel.org/patchwork/patch/1272290/ Just keep the kzalloc() part of the patch, this portion makes no sense to me. But if you REALLY want to get it correct, call dev_set_name() instead please, as that is what it is there for. thanks, greg k-h
Re: Re: (3) [PATCH v2] input: add 2 kind of switch
On Fri, Oct 30, 2020 at 08:59:18PM +0900, Jungrae Kim wrote: > > On Fri, Oct 30, 2020 at 08:28:12PM +0900, Jungrae Kim wrote: > > > > On Fri, Oct 30, 2020 at 01:39:16PM +0900, HyungJae Im wrote: > > > > > Hello, This is Hyungjae Im from Samsung Electronics. > > > > > Let me answer your questions inline. > > > > > > > > > > >On Thu, Oct 29, 2020 at 10:27:47PM +0900, HyungJae Im wrote: > > > > > >> From: "hj2.im" > > > > > >> Date: Thu, 29 Oct 2020 22:11:24 +0900 > > > > > >> Subject: [PATCH v2] input: add 2 kind of switch > > > > > > > > > > > >Why is this in the body of that patch? > > > > > > > > > > I read "how to send your first kernel patch", but still making so > > > > > many mistakes. > > > > > I will be cautious with this. > > > > > > > > > > >> > > > > > >> We need support to various accessories on the device, > > > > > >> some switch does not exist in switch list. > > > > > >> So added switch for the following purpose. > > > > > >> > > > > > >> SW_COVER_ATTACHED is for the checking the cover > > > > > >> attached or not on the device. SW_EXT_PEN_ATTACHED is for the > > > > > >> checking the external pen attached or not on the device > > > > > > > > > > > >You didn't answer the previous question as to why the existing > > > > > >values do > > > > > >not work for you instead of having to create new ones? > > > > > > > > > > I think I should clarify this part the most for this review. > > > > > As you know, new added events both has similar existing events, > > > > > but it has to operate separately. > > > > > > > > > > First, SW_COVER_ATTACHED is similar with SW_MACHINE_COVER. > > > > > We need two events for our cover interaction. > > > > > One is to detect if flip cover is open/closed(covers screen or not), > > > > > and one is for detecting if cover is attached(detect if device is > > > > >put into cover). > > > > > With the second event, we send event for attachment and start > > > > >authentication > > > > > distinguishing if it was Samsung made cover. > > > > > > > > > > Second, SW_EXT_PEN_ATTACHED detects if pen is attached externally on > > > > >tablet models. > > > > > It is different with SW_PEN_INSERTED since this is detecting pens > > > > >like our NOTE series. > > > > > SW_EXT_PEN_ATTACHED has an unique role to set wacom tuning table > > > > >differently > > > > > while pen is attached/detached. > > > > > > > > All of that needs to go in the changelog text for the individual patches > > > > when you submit them. > > > > > > > > But as Dmitry pointed out, it doesn't look like either of these drivers > > > > are needed at all, just use the gpio-keys driver instead. > > > > > > > > thanks, > > > > > > > > greg k-h > > > > > > Can you accept V1 patch? or need to add a change of device tree? > > > > What is "v1" patch? Do you have a pointer to it on lore.kernel.org? > > > > > Please let me know what do I do now. > > > > What is wrong with just using a device tree entry for the gpio-keys > > driver instead? > > > > thanks, > > > > greg k-h > > V1 Patch : > https://lore.kernel.org/lkml/20201021031216epcms1p556d8d7d5d763ec47f67cd8cbe3972935@epcms1p5/ As I said there, that patch is not acceptable for style reasons alone, nothing we can do with that unless it is fixed, right? > I think do not need modify gpio_keys. And I`m not sure device tree need to > added to patch. No, you don't need to modify it, just use it. So what exactly is the issue anymore? Just use the gpio-keys driver and all should be fine, right? thanks, greg k-h
Re: (3) [PATCH v2] input: add 2 kind of switch
On Fri, Oct 30, 2020 at 08:28:12PM +0900, Jungrae Kim wrote: > > On Fri, Oct 30, 2020 at 01:39:16PM +0900, HyungJae Im wrote: > > > Hello, This is Hyungjae Im from Samsung Electronics. > > > Let me answer your questions inline. > > > > > > >On Thu, Oct 29, 2020 at 10:27:47PM +0900, HyungJae Im wrote: > > > >> From: "hj2.im" > > > >> Date: Thu, 29 Oct 2020 22:11:24 +0900 > > > >> Subject: [PATCH v2] input: add 2 kind of switch > > > > > > > >Why is this in the body of that patch? > > > > > > I read "how to send your first kernel patch", but still making so many > > > mistakes. > > > I will be cautious with this. > > > > > > >> > > > >> We need support to various accessories on the device, > > > >> some switch does not exist in switch list. > > > >> So added switch for the following purpose. > > > >> > > > >> SW_COVER_ATTACHED is for the checking the cover > > > >> attached or not on the device. SW_EXT_PEN_ATTACHED is for the > > > >> checking the external pen attached or not on the device > > > > > > > >You didn't answer the previous question as to why the existing values do > > > >not work for you instead of having to create new ones? > > > > > > I think I should clarify this part the most for this review. > > > As you know, new added events both has similar existing events, > > > but it has to operate separately. > > > > > > First, SW_COVER_ATTACHED is similar with SW_MACHINE_COVER. > > > We need two events for our cover interaction. > > > One is to detect if flip cover is open/closed(covers screen or not), > > > and one is for detecting if cover is attached(detect if device is put > > >into cover). > > > With the second event, we send event for attachment and start > > >authentication > > > distinguishing if it was Samsung made cover. > > > > > > Second, SW_EXT_PEN_ATTACHED detects if pen is attached externally on > > >tablet models. > > > It is different with SW_PEN_INSERTED since this is detecting pens like > > >our NOTE series. > > > SW_EXT_PEN_ATTACHED has an unique role to set wacom tuning table > > >differently > > > while pen is attached/detached. > > > > All of that needs to go in the changelog text for the individual patches > > when you submit them. > > > > But as Dmitry pointed out, it doesn't look like either of these drivers > > are needed at all, just use the gpio-keys driver instead. > > > > thanks, > > > > greg k-h > > Can you accept V1 patch? or need to add a change of device tree? What is "v1" patch? Do you have a pointer to it on lore.kernel.org? > Please let me know what do I do now. What is wrong with just using a device tree entry for the gpio-keys driver instead? thanks, greg k-h
Re: (2) [PATCH v2] input: add 2 kind of switch
On Fri, Oct 30, 2020 at 01:39:16PM +0900, HyungJae Im wrote: > Hello, This is Hyungjae Im from Samsung Electronics. > Let me answer your questions inline. > > >On Thu, Oct 29, 2020 at 10:27:47PM +0900, HyungJae Im wrote: > >> From: "hj2.im" > >> Date: Thu, 29 Oct 2020 22:11:24 +0900 > >> Subject: [PATCH v2] input: add 2 kind of switch > > > >Why is this in the body of that patch? > > I read "how to send your first kernel patch", but still making so many > mistakes. > I will be cautious with this. > > >> > >> We need support to various accessories on the device, > >> some switch does not exist in switch list. > >> So added switch for the following purpose. > >> > >> SW_COVER_ATTACHED is for the checking the cover > >> attached or not on the device. SW_EXT_PEN_ATTACHED is for the > >> checking the external pen attached or not on the device > > > >You didn't answer the previous question as to why the existing values do > >not work for you instead of having to create new ones? > > I think I should clarify this part the most for this review. > As you know, new added events both has similar existing events, > but it has to operate separately. > > First, SW_COVER_ATTACHED is similar with SW_MACHINE_COVER. > We need two events for our cover interaction. > One is to detect if flip cover is open/closed(covers screen or not), > and one is for detecting if cover is attached(detect if device is put into > cover). > With the second event, we send event for attachment and start authentication > distinguishing if it was Samsung made cover. > > Second, SW_EXT_PEN_ATTACHED detects if pen is attached externally on tablet > models. > It is different with SW_PEN_INSERTED since this is detecting pens like our > NOTE series. > SW_EXT_PEN_ATTACHED has an unique role to set wacom tuning table differently > while pen is attached/detached. All of that needs to go in the changelog text for the individual patches when you submit them. But as Dmitry pointed out, it doesn't look like either of these drivers are needed at all, just use the gpio-keys driver instead. thanks, greg k-h
Re: [PATCH] Input: add switch event(SW_EXT_PEN_ATTACHED)
On Fri, Oct 30, 2020 at 03:27:40PM +0900, HyungJae Im wrote:
> We need support to various accessories on the device,
> some requiring switch does not exist in switch list.
> So added switch for the following purpose.
>
> SW_EXT_PEN_ATTACHED is for the checking the external pen
> attached or not on the device. We also added driver
> that uses such event.
>
> Signed-off-by: Hyungjae Im
> ---
> drivers/input/Kconfig | 12 ++
> drivers/input/Makefile | 1 +
> drivers/input/ext_pen_detect.c | 237 +
> include/linux/mod_devicetable.h| 2 +-
> include/uapi/linux/input-event-codes.h | 3 +-
> 5 files changed, 253 insertions(+), 2 deletions(-)
> create mode 100644 drivers/input/ext_pen_detect.c
>
> diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig
> index ba5e7444c547..5d6d15c8f7e7 100644
> --- a/drivers/input/Kconfig
> +++ b/drivers/input/Kconfig
> @@ -197,6 +197,18 @@ config INPUT_COVER_DETECT
> To compile this driver as a module, choose M here: the
> module will be called cover_detect.
>
> +config INPUT_EXT_PEN_DETECT
> + tristate "Enable external pen attach detection"
> + help
> + Say Y here to enable external pen detection
> + and send a event when external pen is attached/detached.
> + Active gpio state is low and active event value is 0.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ext_pen_detect.
> +
> comment "Input Device Drivers"
>
> source "drivers/input/keyboard/Kconfig"
> diff --git a/drivers/input/Makefile b/drivers/input/Makefile
> index fc8dd9091821..0ccf02e34557 100644
> --- a/drivers/input/Makefile
> +++ b/drivers/input/Makefile
> @@ -31,3 +31,4 @@ obj-$(CONFIG_INPUT_APMPOWER)+= apm-power.o
> obj-$(CONFIG_RMI4_CORE) += rmi4/
>
> obj-$(CONFIG_INPUT_COVER_DETECT)+= cover_detect.o
> +obj-$(CONFIG_INPUT_EXT_PEN_DETECT)+= ext_pen_detect.o
> diff --git a/drivers/input/ext_pen_detect.c b/drivers/input/ext_pen_detect.c
> new file mode 100644
> index ..9a0d106e49f8
> --- /dev/null
> +++ b/drivers/input/ext_pen_detect.c
> @@ -0,0 +1,237 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Support detection pen attachment externally on device
> + *
> + * Copyright (C) 2020 Samsung Electronics Co. Ltd. All Rights Reserved.
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +struct ext_pen_detect_drvdata {
> + struct input_dev *input;
> + struct delayed_work ext_pen_detect_dwork;
> + struct wakeup_source *ws;
> + int gpio_ext_pen_detect;
> + int irq_ext_pen_detect;
> +};
> +
> +static void ext_pen_detect_work(struct work_struct *work)
> +{
> + struct ext_pen_detect_drvdata *ddata =
> + container_of(work, struct ext_pen_detect_drvdata,
> + ext_pen_detect_dwork.work);
> + bool ext_pen_status;
> +
> + ext_pen_status = gpio_get_value(ddata->gpio_ext_pen_detect);
> +
> + input_report_switch(ddata->input,
> + SW_EXT_PEN_ATTACHED, ext_pen_status);
As this is just a gpio device, again, why is this needed and you can't
just use the gpio_keys driver instead?
Why does this have to be a new driver?
thanks,
greg k-h
Re: [PATCH v2] input: add 2 kind of switch
On Thu, Oct 29, 2020 at 10:27:47PM +0900, HyungJae Im wrote: > From: "hj2.im" > Date: Thu, 29 Oct 2020 22:11:24 +0900 > Subject: [PATCH v2] input: add 2 kind of switch Why is this in the body of that patch? > > We need support to various accessories on the device, > some switch does not exist in switch list. > So added switch for the following purpose. > > SW_COVER_ATTACHED is for the checking the cover > attached or not on the device. SW_EXT_PEN_ATTACHED is for the > checking the external pen attached or not on the device You didn't answer the previous question as to why the existing values do not work for you instead of having to create new ones? > > Signed-off-by: Hyungjae Im > --- > drivers/input/Kconfig | 20 ++ > drivers/input/Makefile | 3 + > drivers/input/cover_detect.c | 242 > drivers/input/ext_pen_detect.c | 243 + > include/linux/mod_devicetable.h| 2 +- > include/uapi/linux/input-event-codes.h | 4 +- > 6 files changed, 512 insertions(+), 2 deletions(-) > create mode 100644 drivers/input/cover_detect.c > create mode 100644 drivers/input/ext_pen_detect.c If this is v2, what changed from v1? And this is 2 different drivers, it should be 2 different patches at the least, right?> > > diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig > index 1efd3154b68d..df902f4a549e 100644 > --- a/drivers/input/Kconfig > +++ b/drivers/input/Kconfig > @@ -185,6 +185,26 @@ config INPUT_APMPOWER > To compile this driver as a module, choose M here: the > module will be called apm-power. > > +config COVER_DETECT INPUT_COVER_DETECT? > + tristate "Enable cover attach detection" > + default n "default n" is always the default, no need for this here. > + help > + Say Y here to enable cover attach detection > + and send a event when cover is attached/detached. > + Active gpio state is low and active event value is 0. > + > + If unsure, say N. What is the module name? > + > +config EXT_PEN_DETECT INPUT_EXT_PEN_DETECT? > + tristate "Enable external pen attach detection" > + default n No default n. > + help > + Say Y here to enable external pen attach detection > + and send a event when external pen is attached/detached. > + Active gpio state is low and active event value is 0. > + > + If unsure, say N. What is the module name? > + > comment "Input Device Drivers" > > source "drivers/input/keyboard/Kconfig" > diff --git a/drivers/input/Makefile b/drivers/input/Makefile > index e35650930371..31ee1f2d2e21 100644 > --- a/drivers/input/Makefile > +++ b/drivers/input/Makefile > @@ -29,3 +29,6 @@ obj-$(CONFIG_INPUT_MISC)+= misc/ > obj-$(CONFIG_INPUT_APMPOWER) += apm-power.o > > obj-$(CONFIG_RMI4_CORE) += rmi4/ > + > +obj-$(CONFIG_COVER_DETECT) += cover_detect.o > +obj-$(CONFIG_EXT_PEN_DETECT) += ext_pen_detect.o > diff --git a/drivers/input/cover_detect.c b/drivers/input/cover_detect.c > new file mode 100644 > index ..4d3d68c616ec > --- /dev/null > +++ b/drivers/input/cover_detect.c > @@ -0,0 +1,242 @@ > +/* > + * Copyright (C) 2015 Samsung Electronics Co. Ltd. All Rights Reserved. Please use a SPDX line, and no need for this: > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. That can be removed. Also your copyright line is wrong, unless you really have not touched this file in 5 years. Same comments on the other file. thanks, greg k-h
Re: [PATCH v2] input: add 2 kind of switch
On Thu, Oct 29, 2020 at 01:41:57PM +, Barnabás Pőcze wrote:
> Hi
>
> > [...]
> > diff --git a/include/linux/mod_devicetable.h
> > b/include/linux/mod_devicetable.h
> > index 5b08a473cdba..897f5a3e7721 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -320,7 +320,7 @@ struct pcmcia_device_id {
> > #define INPUT_DEVICE_ID_LED_MAX0x0f
> > #define INPUT_DEVICE_ID_SND_MAX0x07
> > #define INPUT_DEVICE_ID_FF_MAX 0x7f
> > -#define INPUT_DEVICE_ID_SW_MAX 0x10
> > +#define INPUT_DEVICE_ID_SW_MAX 0x12
> > #define INPUT_DEVICE_ID_PROP_MAX 0x1f
> >
> > #define INPUT_DEVICE_ID_MATCH_BUS 1
> > diff --git a/include/uapi/linux/input-event-codes.h
> > b/include/uapi/linux/input-event-codes.h
> > index 0c2e27d28e0a..8ca2acee1f92 100644
> > --- a/include/uapi/linux/input-event-codes.h
> > +++ b/include/uapi/linux/input-event-codes.h
> > @@ -889,7 +889,9 @@
> > #define SW_MUTE_DEVICE 0x0e /* set = device disabled */
> > #define SW_PEN_INSERTED0x0f /* set = pen inserted */
> > #define SW_MACHINE_COVER 0x10 /* set = cover closed */
> > -#define SW_MAX 0x10
> > +#define SW_COVER_ATTACHED 0x11 /* set = cover attached */
> > +#define SW_EXT_PEN_ATTACHED0x12 /* set = external pen attached */
> > +#define SW_MAX 0x12
> > #define SW_CNT (SW_MAX+1)
> > [...]
>
> This part of the patch conflicts with another one:
> https://lore.kernel.org/linux-input/20201026144512.621479-1-markpear...@lenovo.com/
Is that merged? If not, it's fine as-is until then, and someone has to
pick to go first :)
But, most importantly, the questions asked last time about this patch
have not been addressed at all :(
thanks,
greg k-h
Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
On Thu, Oct 29, 2020 at 11:07:27AM +0100, Vincent Whitchurch wrote: > On Wed, Oct 28, 2020 at 04:50:36PM +0100, Arnd Bergmann wrote: > > I think we should try to do something on top of the PCIe endpoint subsystem > > to make it work across arbitrary combinations of host and device > > implementations, > > and provide a superset of what the MIC driver, (out-of-tree) Bluefield > > endpoint > > driver, and the NTB subsystem as well as a couple of others used to do, > > each of them tunneling block/network/serial/... over a PCIe link of some > > sort, usually with virtio. > > VOP is not PCIe-specific (as demonstrated by the vop-loopback patches I > posted a while ago [1]), and it would be a shame for a replacement to be > tied to the PCIe endpoint subsystem. There are many SOCs out there > which have multiple Linux-capable processors without cache-coherency > between them. VOP is (or should I say was since I guess it's being > deleted) the closest we have in mainline to easily get generic virtio > (and not just rpmsg) running between these kind of Linux instances. If > a new replacement framework were to be PCIe-exclusive then we'd have to > invent one more framework for non-PCIe links to do pretty much the same > thing. > > [1] > https://lore.kernel.org/lkml/20190403104746.16063-1-vincent.whitchu...@axis.com/ If this works well, please restore the existing code and move it to a new directory and we can take it from there, right? thanks, greg k-h
Re: [PATCH char-misc-next 1/1] misc: mic: remove the MIC drivers
On Wed, Oct 28, 2020 at 05:22:01PM +, Dutt, Sudeep wrote: > On Wed, 2020-10-28 at 06:54 +0100, Greg Kroah-Hartman wrote: > > On Tue, Oct 27, 2020 at 08:14:15PM -0700, Sudeep Dutt wrote: > > > This patch removes the MIC drivers from the kernel tree > > > since the corresponding devices have been discontinued. > > > > Does "discontinued" mean "never shipped a device so no one has access > > to > > this hardware anymore", or does it mean "we stopped shipping devices > > and > > there are customers with this?" > > Hi Greg, > > We are not aware of any customers of the upstreamed MIC drivers. The > drivers were upstreamed primarily to lay a foundation for enabling the > next generation MIC devices which did not ship. Ok, thanks for the explanation. > > > > Removing the dma and char-misc changes in one patch and > > > merging via the char-misc tree is best to avoid any > > > potential build breakage. > > > > > > Cc: Nikhil Rao > > > Reviewed-by: Ashutosh Dixit > > > Signed-off-by: Sudeep Dutt > > > > I like deleting code, can this go into 5.10-final? > > Yes, we would prefer this goes into v5.10. I am hoping you can carry > the Ack from Vinod and the Reviewed-by from Sherry but I can resend the > patch with those updates in the commit message if required. I did > verify that this patch passes allmodconfig and allyesconfig builds with > your latest char-misc-next tree. I can pick them up automatically, no worries, thanks! greg k-h
Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
On Mon, Oct 26, 2020 at 03:04:45AM +, Sherry Sun wrote: > Hi Greg & Christoph, > > > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > > ring > > > > On Thu, Oct 22, 2020 at 01:06:36PM +0800, Sherry Sun wrote: > > > We don't need to allocate and reassign the used ring here and remove > > > the used_address_updated flag.Since RC has allocated the entire vring, > > > including the used ring. Simply add the corresponding offset can get > > > the used ring address. > > > > Someone needs to verify this vs the existing intel implementations. > > Hi Greg, @gre...@linuxfoundation.org, sorry I don't have the Intel MIC > devices so cannot test it, do you know anyone who can help test this patch > changes on the Intel MIC platform? Thanks. Why not ask the authors/maintainers of that code? greg k-h
Re: [PATCH] FROMLIST: input: add 2 kind of switch
On Wed, Oct 21, 2020 at 12:12:16PM +0900, HyungJae Im wrote:
> >From ec9859ee01b7bc0e04255971e0fe97348847dab7 Mon Sep 17 00:00:00 2001
You sent this 3 times, why?
And why is this in the body of the email, have you read the "how to send
your first kernel patch" document at kernelnewbies.org?
> From: "hj2.im"
> Date: Tue, 20 Oct 2020 16:57:04 +0900
> Subject: [PATCH] FROMLIST: input: add 2 kind of switch
What does "FROMLIST:" mean?
>
> We need support to various accessories on the device,
> some switch does not exist in switch list.
> So added switch for the following purpose.
>
> SW_COVER_ATTACHED is for the checking the cover
> attached or not on the device. SW_EXT_PEN_ATTACHED is for the
> checking the external pen attached or not on the device
>
> Signed-off-by: hj2.im
As per the kernel documentation, you need to use your real name here,
please do so.
> ---
> include/linux/mod_devicetable.h| 2 +-
> include/uapi/linux/input-event-codes.h | 4 +++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 5b08a473cdba..897f5a3e7721 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -320,7 +320,7 @@ struct pcmcia_device_id {
> #define INPUT_DEVICE_ID_LED_MAX 0x0f
> #define INPUT_DEVICE_ID_SND_MAX 0x07
> #define INPUT_DEVICE_ID_FF_MAX 0x7f
> -#define INPUT_DEVICE_ID_SW_MAX 0x10
> +#define INPUT_DEVICE_ID_SW_MAX 0x12
> #define INPUT_DEVICE_ID_PROP_MAX 0x1f
>
> #define INPUT_DEVICE_ID_MATCH_BUS1
> diff --git a/include/uapi/linux/input-event-codes.h
> b/include/uapi/linux/input-event-codes.h
> index 0c2e27d28e0a..8ca2acee1f92 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -889,7 +889,9 @@
> #define SW_MUTE_DEVICE 0x0e /* set = device disabled */
> #define SW_PEN_INSERTED 0x0f /* set = pen inserted */
> #define SW_MACHINE_COVER 0x10 /* set = cover closed */
> -#define SW_MAX 0x10
> +#define SW_COVER_ATTACHED0x11 /* set = cover attached */
> +#define SW_EXT_PEN_ATTACHED 0x12 /* set = external pen attached */
Is there an in-kernel user for these values anywhere? Please submit
this patch along with the users at the same time, otherwise this change
makes no sense at all.
thanks,
greg k-h
Re: [PATCH 1/2] fpga: dfl: add driver_override support
On Mon, Oct 19, 2020 at 03:50:32PM +0800, Xu Yilun wrote:
> On Mon, Oct 19, 2020 at 03:46:23PM +0800, Wu, Hao wrote:
> > > On Fri, Oct 16, 2020 at 09:21:50AM -0700, Tom Rix wrote:
> > > >
> > > > On 10/15/20 11:02 PM, Xu Yilun wrote:
> > > > > Add support for overriding the default matching of a dfl device to a
> > > > > dfl
> > > > > driver. It follows the same way that can be used for PCI and platform
> > > > > devices. This patch adds the 'driver_override' sysfs file.
> > > > >
> > > > > Signed-off-by: Xu Yilun
> > > > > ---
> > > > > Documentation/ABI/testing/sysfs-bus-dfl | 28 ++---
> > > > > drivers/fpga/dfl.c | 54
> > > -
> > > > > include/linux/dfl.h | 2 ++
> > > > > 3 files changed, 79 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl
> > > b/Documentation/ABI/testing/sysfs-bus-dfl
> > > > > index 23543be..db7e8d3 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-bus-dfl
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> > > > > @@ -1,15 +1,35 @@
> > > > > What:/sys/bus/dfl/devices/dfl_dev.X/type
> > > > > -Date:Aug 2020
> > > > > -KernelVersion:5.10
> > > > > +Date:Oct 2020
> > > > > +KernelVersion:5.11
> > > > > Contact:Xu Yilun
> > > > > Description:Read-only. It returns type of DFL FIU of the device.
> > > Now DFL
> > > > > supports 2 FIU types, 0 for FME, 1 for PORT.
> > > > > Format: 0x%x
> > > > >
> > > > > What:/sys/bus/dfl/devices/dfl_dev.X/feature_id
> > > > > -Date:Aug 2020
> > > > > -KernelVersion:5.10
> > > > > +Date:Oct 2020
> > > > > +KernelVersion:5.11
> > > > > Contact:Xu Yilun
> > > > > Description:Read-only. It returns feature identifier local to its DFL
> > > FIU
> > > > > type.
> > > > > Format: 0x%x
> > > >
> > > > These updates, do not match the comment.
> > > >
> > > > Consider splitting this out.
> > >
> > > I'm sorry it's a typo. The above code should not be changed.
> > >
> > > >
> > > > > +
> > > > > +What: /sys/bus/dfl/devices/.../driver_override
> > > > > +Date: Oct 2020
> > > > > +KernelVersion: 5.11
> > > > > +Contact:Xu Yilun
> > > > I am looking at description and trying to make it consistent with
> > > > sysfs-bus-
> > > pci
> > > > > +Description:This file allows the driver for a device to be
> > > > > specified.
> > > >
> > > > 'to be specified which will override the standard dfl bus feature id to
> > > > driver
> > > mapping.'
> > >
> > > Yes, it could be improved.
> > >
> > > Actually now it is the "type" and "feature id" matching, the 2 fields
> > > are defined for dfl_driver.id_table. In future for dfl v1, it may be
> > > GUID matching, which will be added to id_table. So how about we make it
> > > more generic:
> > >
> > > 'to be specified which will override the standard ID table matching.'
> > >
> > > >
> > > >
> > > > > When
> > > > > +specified, only a driver with a name matching the
> > > > > value written
> > > > > +to driver_override will have an opportunity to bind
> > > > > to the
> > > > > +device. The override is specified by writing a
> > > > > string to the
> > > > > +driver_override file (echo dfl-uio-pdev >
> > > > > driver_override) and
> > > > > +may be cleared with an empty string (echo >
> > > > > driver_override).
> > > > > +This returns the device to standard matching rules
> > > > > binding.
> > > > > +Writing to driver_override does not automatically
> > > > > unbind the
> > > > > +device from its current driver or make any attempt to
> > > > > +automatically load the specified driver. If no
> > > > > driver with a
> > > > > +matching name is currently loaded in the kernel, the
> > > > > device
> > > > > +will not bind to any driver. This also allows
> > > > > devices to
> > > > > +opt-out of driver binding using a driver_override
> > > > > name such as
> > > > > +"none". Only a single driver may be specified in
> > > > > the override,
> > > > > +there is no support for parsing delimiters.
> > > > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > > > > index 511b20f..bc35750 100644
> > > > > --- a/drivers/fpga/dfl.c
> > > > > +++ b/drivers/fpga/dfl.c
> > > > > @@ -262,6 +262,10 @@ static int dfl_bus_match(struct device *dev,
> > > struct device_driver *drv)
> > > > > struct dfl_driver *ddrv = to_dfl_drv(drv);
> > > > > const struct dfl_device_id *id_entry;
> > > > >
> > > > > +/* When driver_override is set, only bind to the matching driver */
> > > > > +if (ddev->driver_override)
> > > > > +return !strcmp(ddev->driver_override, drv->name);
> > > > > +
> > > > > id_entry = ddrv->id_table;
> > > > > if (id_entry) {
> > > > > while (id_entry->feature_id) {
> > > > > @@ -303,6
Re: [PATCH] drivers/virt: vmgenid: add vm generation id driver
On Fri, Oct 16, 2020 at 02:33:15PM +, Catangiu, Adrian Costin wrote:
> +config VMGENID
> + tristate "Virtual Machine Generation ID driver"
> + depends on ACPI
> + default M
Unless this is required to boot a machine, this should be removed.
> + help
> + This is a Virtual Machine Generation ID driver which provides
> + a virtual machine unique identifier. The provided UUID can be
> + watched through the FS interface exposed by this driver, and
> + thus can provide notifications for VM snapshot or cloning events.
Where is the uuid exposed in the filesystem?
> + This enables applications and libraries that store or cache
> + sensitive information, to know that they need to regenerate it
> + after process memory has been exposed to potential copying.
Module name?
> +
> config FSL_HV_MANAGER
> tristate "Freescale hypervisor management driver"
> depends on FSL_SOC
> diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
> index fd33124..a1f8dcc 100644
> --- a/drivers/virt/Makefile
> +++ b/drivers/virt/Makefile
> @@ -4,4 +4,5 @@
> #
>
> obj-$(CONFIG_FSL_HV_MANAGER) += fsl_hypervisor.o
> +obj-$(CONFIG_VMGENID)+= vmgenid.o
> obj-y+= vboxguest/
> diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
> new file mode 100644
> index 000..d314c72
> --- /dev/null
> +++ b/drivers/virt/vmgenid.c
> @@ -0,0 +1,419 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Virtual Machine Generation ID driver
> + *
> + * Copyright (C) 2018 Red Hat Inc, Copyright (C) 2020 Amazon.com Inc
That's not how you write copyright lines, please fix up.
> + * All rights reserved.
> + * Authors:
> + * Adrian Catangiu
> + * Or Idgar
> + * Gal Hammer
> + *
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define DEV_NAME "vmgenid"
> +ACPI_MODULE_NAME(DEV_NAME);
> +
> +struct dev_data {
> + struct cdev cdev;
> + dev_t dev_id;
> + unsigned long map_buf;
> +
> + void *uuid_iomap;
> + uuid_tuuid;
> + wait_queue_head_t read_wait;
> +
> + atomic_t watchers;
> + atomic_t outdated_watchers;
> + wait_queue_head_t outdated_wait;
> +};
> +
> +struct file_data {
> + struct dev_data *dev_data;
> + uuid_t acked_uuid;
> +};
> +
> +static bool vmgenid_uuid_matches(struct dev_data *priv, uuid_t *uuid)
> +{
> + return !memcmp(uuid, >uuid, sizeof(uuid_t));
> +}
> +
> +static void vmgenid_put_outdated_watchers(struct dev_data *priv)
> +{
> + if (atomic_dec_and_test(>outdated_watchers))
> + wake_up_interruptible(>outdated_wait);
> +}
> +
> +static int vmgenid_open(struct inode *inode, struct file *file)
> +{
> + struct dev_data *priv =
> + container_of(inode->i_cdev, struct dev_data, cdev);
> + struct file_data *file_data =
> + kzalloc(sizeof(struct file_data), GFP_KERNEL);
> +
> + if (!file_data)
> + return -ENOMEM;
> +
> + file_data->acked_uuid = priv->uuid;
> + file_data->dev_data = priv;
> +
> + file->private_data = file_data;
> + atomic_inc(>watchers);
> +
> + return 0;
> +}
> +
> +static int vmgenid_close(struct inode *inode, struct file *file)
> +{
> + struct file_data *file_data = (struct file_data *) file->private_data;
> + struct dev_data *priv = file_data->dev_data;
> +
> + if (!vmgenid_uuid_matches(priv, _data->acked_uuid))
> + vmgenid_put_outdated_watchers(priv);
> + atomic_dec(>watchers);
> + kfree(file->private_data);
> +
> + return 0;
> +}
> +
> +static ssize_t
> +vmgenid_read(struct file *file, char __user *ubuf, size_t nbytes, loff_t
> *ppos)
> +{
> + struct file_data *file_data =
> + (struct file_data *) file->private_data;
> + struct dev_data *priv = file_data->dev_data;
> + ssize_t ret;
> +
> + if (nbytes == 0)
> + return 0;
> + /* disallow partial UUID reads */
> + if (nbytes < sizeof(uuid_t))
> + return -EINVAL;
> + nbytes = sizeof(uuid_t);
> +
> + if (vmgenid_uuid_matches(priv, _data->acked_uuid)) {
> + if (file->f_flags & O_NONBLOCK)
> + return -EAGAIN;
> + ret = wait_event_interruptible(
> + priv->read_wait,
> + !vmgenid_uuid_matches(priv, _data->acked_uuid)
> + );
> + if (ret)
> + return ret;
> + }
> +
> + ret = copy_to_user(ubuf, >uuid, nbytes);
> + if (ret)
> + return -EFAULT;
> +
> + return nbytes;
> +}
> +
> +static ssize_t vmgenid_write(struct file *file, const char __user *ubuf,
> + size_t count, loff_t *ppos)
> +{
> + struct file_data *file_data = (struct file_data *) file->private_data;
> + struct
Re: [PATCH V2 0/2] Add module autoloading support for vop and cosm driver
On Tue, Oct 13, 2020 at 08:52:01AM +, Sherry Sun wrote: > Gentle ping It's the merge window, sorry, this fell through the cracks before that happened. Please resubmit once 5.10-rc1 comes out. thanks, greg k-h
Re: LTS couple perf test and perf top fixes
On Fri, Oct 09, 2020 at 11:28:09AM +, Rantala, Tommi T. (Nokia - FI/Espoo) wrote: > Hi Greg, Sasha, > > Can you pick this to 5.4: > > commit dbd660e6b2884b864d2642d930a163d3bcebe4be > Author: Tommi Rantala > Date: Thu Apr 23 14:53:40 2020 +0300 > > perf test session topology: Fix data path > > > And this to 5.4 and older LTS trees too: > > commit 29b4f5f188571c112713c35cc87eefb46efee612 > Author: Tommi Rantala > Date: Thu Mar 5 10:37:12 2020 +0200 > > perf top: Fix stdio interface input handling with glibc 2.28+ > Now queued up, thanks. greg k-h
Re: [RFC] debugfs: protect against rmmod while files are open
On Fri, Oct 09, 2020 at 10:56:16AM +, David Laight wrote: > From: Johannes Berg > > Sent: 09 October 2020 11:48 > > > > On Fri, 2020-10-09 at 12:41 +0200, Johannes Berg wrote: > > > > > If the fops doesn't have a release method, we don't even need > > > to keep a reference to the real_fops, we can just fops_put() > > > them already in debugfs remove, and a later full_proxy_release() > > > won't call anything anyway - this just crashed/UAFed because it > > > used real_fops, not because there was actually a (now invalid) > > > release() method. > > > > I actually implemented something a bit better than what I described - we > > never need a reference to the real_fops for the release method alone, > > and that means if the release method is in the kernel image, rather than > > a module, it can still be called. > > > > That together should reduce the ~117 places you changed in the large > > patchset to around a handful. > > Is there an equivalent problem for normal cdev opens > in any modules? What does cdev have to do with debugfs?
Re: [PATCH] usb: host: ehci-sched: avoid possible NULL dereference
On Mon, Oct 05, 2020 at 11:19:02PM +, Harley A.W. Lorenzo wrote: > On Monday, October 5, 2020 5:31 PM, Sudip Mukherjee > wrote: > > > find_tt() can return NULL or the error value in ERR_PTR() and > > dereferencing the return value without checking for the error can > > lead to a possible dereference of NULL pointer or ERR_PTR(). > > Looks fine to me. There is in fact no checks of the return value > before a dereference here, and this solves that. > > Reviewed-by: Harley A.W. Lorenzo ' there. thanks, greg k-h
Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving parameter
On Wed, Sep 16, 2020 at 08:32:26AM -0500, Bjorn Helgaas wrote: > > So is it ok to take this patch now, or does it need to be changed any? > > Yes, it's OK with me if you take this patch. > > The ASPM hardware feature is designed to work without any driver > support. It does need to be configured, which involves both the > device and the upstream bridge, so it should be done by the BIOS or > the PCI core. There are a few drivers (amdgpu, radeon, hfi1, e1000e, > iwlegacy, ath10k, ath9k, mt76, rtlwifi, rtw88, and these rts > cardreader drivers) that do it themselves, incorrectly. > > But this particular patch only *reads* the ASPM control registers, > without writing them, so it shouldn't make anything worse. Ok, thanks for the review, now queued up. greg k-h
Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving parameter
On Tue, Sep 15, 2020 at 10:28:11AM -0500, Bjorn Helgaas wrote: > On Tue, Sep 15, 2020 at 08:24:50AM +, 吳昊澄 Ricky wrote: > > > -Original Message- > > > From: Bjorn Helgaas [mailto:helg...@kernel.org] > > > Sent: Friday, September 11, 2020 10:56 PM > > > To: 吳昊澄 Ricky > > > Cc: a...@arndb.de; gre...@linuxfoundation.org; bhelg...@google.com; > > > ulf.hans...@linaro.org; rui_f...@realsil.com.cn; > > > linux-kernel@vger.kernel.org; > > > puranja...@gmail.com; linux-...@vger.kernel.org; > > > vailbhavgupt...@gamail.com > > > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and > > > fix driving > > > parameter > > > > > > On Fri, Sep 11, 2020 at 08:18:22AM +, 吳昊澄 Ricky wrote: > > > > > -Original Message- > > > > > From: Bjorn Helgaas [mailto:helg...@kernel.org] > > > > > Sent: Thursday, September 10, 2020 1:44 AM > > > > > To: 吳昊澄 Ricky > > > > > Cc: a...@arndb.de; gre...@linuxfoundation.org; bhelg...@google.com; > > > > > ulf.hans...@linaro.org; rui_f...@realsil.com.cn; > > > linux-kernel@vger.kernel.org; > > > > > puranja...@gmail.com; linux-...@vger.kernel.org; > > > > > vailbhavgupt...@gamail.com > > > > > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions > > > > > and fix > > > driving > > > > > parameter > > > > > > > > > > On Wed, Sep 09, 2020 at 07:10:19AM +, 吳昊澄 Ricky wrote: > > > > > > > -Original Message- > > > > > > > From: Bjorn Helgaas [mailto:helg...@kernel.org] > > > > > > > Sent: Wednesday, September 09, 2020 6:29 AM > > > > > > > To: 吳昊澄 Ricky > > > > > > > Cc: a...@arndb.de; gre...@linuxfoundation.org; > > > bhelg...@google.com; > > > > > > > ulf.hans...@linaro.org; rui_f...@realsil.com.cn; > > > > > linux-kernel@vger.kernel.org; > > > > > > > puranja...@gmail.com; linux-...@vger.kernel.org; > > > > > > > vailbhavgupt...@gamail.com > > > > > > > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving > > > > > > > functions and fix > > > > > driving > > > > > > > parameter > > > > > > > > > > > > > > On Mon, Sep 07, 2020 at 06:07:31PM +0800, ricky...@realtek.com > > > wrote: > > > > > > > > From: Ricky Wu > > > > > > > > > > > > > > > > v4: > > > > > > > > split power down flow and power saving function to two patch > > > > > > > > > > > > > > > > v5: > > > > > > > > fix up modified change under the --- line > > > > > > > > > > > > > > Hehe, this came out *above* the "---" line :) > > > > > > > > > > > > > > > Add rts522a L1 sub-state support > > > > > > > > Save more power on rts5227 rts5249 rts525a rts5260 > > > > > > > > Fix rts5260 driving parameter > > > > > > > > > > > > > > > > Signed-off-by: Ricky Wu > > > > > > > > --- > > > > > > > > drivers/misc/cardreader/rts5227.c | 112 +- > > > > > > > > drivers/misc/cardreader/rts5249.c | 145 > > > > > > > - > > > > > > > > drivers/misc/cardreader/rts5260.c | 28 +++--- > > > > > > > > drivers/misc/cardreader/rtsx_pcr.h | 17 > > > > > > > > 4 files changed, 283 insertions(+), 19 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/misc/cardreader/rts5227.c > > > > > > > b/drivers/misc/cardreader/rts5227.c > > > > > > > > index 747391e3fb5d..8859011672cb 100644 > > > > > > > > --- a/drivers/misc/cardreader/rts5227.c > > > > > > > > +++ b/drivers/misc/cardreader/rts5227.c > > > > > > > > @@ -72,15 +72,80 @@ static void > > > rts5227_fetch_vendor_settings(struct > > > > > > > rtsx_pcr *pcr) > > > > > > > > > > > > > > > > pci_read_config_dword(pdev, PCR_SETTING_REG2
Re: [PATCH] /dev/zero: also implement ->read
On Sun, Sep 06, 2020 at 08:38:20PM +0200, Pavel Machek wrote: > On Sun 2020-09-06 20:35:38, Christophe Leroy wrote: > > Hi, > > > > Le 06/09/2020 à 20:21, Pavel Machek a écrit : > > >Hi! > > > > > Christophe reported a major speedup due to avoiding the iov_iter > > overhead, so just add this trivial function. Note that /dev/zero > > already implements both an iter and non-iter writes so this just > > makes it more symmetric. > > > > Christophe Leroy > > Signed-off-by: Christoph Hellwig > > >>> > > >>>Tested-by: Christophe Leroy > > >> > > >>Any idea what has happened to make the 'iter' version so bad? > > > > > >Exactly. Also it would be nice to note how the speedup was measured > > >and what the speedup is. > > > > > > > Was measured on an 8xx powerpc running at 132MHz with: > > > > dd if=/dev/zero of=/dev/null count=1M > > > > With the patch, dd displays a throughput of 113.5MB/s > > Without the patch it is 99.9MB/s > > Actually... that does not seem like a huge deal. read(/dev/zero) is > not that common operation. There is nothing wrong with this patch (aside from the sparse warning), and it's in my tree now, so I don't understand complaining about it... greg k-h
Re: y2038 backport to v5.4
On Mon, Aug 17, 2020 at 03:00:24PM +, Roosen Henri wrote: > On Mon, 2020-08-17 at 16:35 +0200, gre...@linuxfoundation.org wrote: > > On Mon, Aug 17, 2020 at 02:15:16PM +, Roosen Henri wrote: > > > On Tue, 2020-06-09 at 16:18 +0200, Arnd Bergmann wrote: > > > > On Tue, Jun 9, 2020 at 2:36 PM Roosen Henri < > > > > henri.roo...@ginzinger.com> wrote: > > > > > Hi Arnd, > > > > > > > > > > I hope you are well and could answer me a quick question. > > > > > > > > > > I've read on the kernel mailing-list that initially there was > > > > > an > > > > > intention to backport the final y2038 patches to v5.4. We're > > > > > currently targeting to use the v5.4 LTS kernel for a project > > > > > which > > > > > should be y2038 compliant. > > > > > > > > > > I couldn't find all of the y2038-endgame patches in the current > > > > > v5.4-stable branch. Are there any patches still required to be > > > > > backported in order for v5.4 to be y2038 compliant, or can the > > > > > remaining patches be ignored (because of only cleanup?)? Else, > > > > > is > > > > > there still an intention to get the v5.4 LTS kernel y2038 > > > > > compliant? > > > > > > > > I don't think there are currently any plans to merge my y2038- > > > > endgame > > > > branch > > > > into the official linux-5.4 lts kernel, but you should be able to > > > > just pull from > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=y2038-endgame > > > > > > > > and get the same results. If you see any problems with that, > > > > please > > > > report > > > > that to me with Cc to the mailing list and perhaps gregkh, so I > > > > can > > > > see if > > > > I can resolve it by rebasing my patches, or if he would like to > > > > merge > > > > the > > > > patches. > > > > > > Pulling the y2038-endgame branch does lead to some conflicts, which > > > are > > > currently still kinda staightforward to solve. > > > > > > However I'd be very interested in getting this branch merged to > > > v5.4, > > > so we don't run into more difficult merge conflicts the coming > > > years > > > where the v5.4-LTS still gets stable updates (Dec, 2025) and > > > possibly > > > to get any related fixes from upstream. > > > > > > @Greg: any chance to get the y2038-endgame merged into v5.4.y? > > > > I have no idea what this really means, and what it entails, but odds > > are, no :) > > I fully understand, thanks for your statement on this. > > > > > Why not just use a newer kernel? Why are you stuck using a 5.4 > > kernel > > for a device that has to live in 2038? That feels very foolish to > > me... > > Oh I agree on that :) It's just that these are currently customer > requirements. Are you sure that customers really understand what they want? Usually they want a well-supported, stable, system. Why do they care about a specific kernel version? That feels odd. Good luck! greg k-h
Re: y2038 backport to v5.4
On Mon, Aug 17, 2020 at 02:15:16PM +, Roosen Henri wrote: > On Tue, 2020-06-09 at 16:18 +0200, Arnd Bergmann wrote: > > On Tue, Jun 9, 2020 at 2:36 PM Roosen Henri < > > henri.roo...@ginzinger.com> wrote: > > > Hi Arnd, > > > > > > I hope you are well and could answer me a quick question. > > > > > > I've read on the kernel mailing-list that initially there was an > > > intention to backport the final y2038 patches to v5.4. We're > > > currently targeting to use the v5.4 LTS kernel for a project which > > > should be y2038 compliant. > > > > > > I couldn't find all of the y2038-endgame patches in the current > > > v5.4-stable branch. Are there any patches still required to be > > > backported in order for v5.4 to be y2038 compliant, or can the > > > remaining patches be ignored (because of only cleanup?)? Else, is > > > there still an intention to get the v5.4 LTS kernel y2038 > > > compliant? > > > > I don't think there are currently any plans to merge my y2038-endgame > > branch > > into the official linux-5.4 lts kernel, but you should be able to > > just pull from > > > > https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=y2038-endgame > > > > and get the same results. If you see any problems with that, please > > report > > that to me with Cc to the mailing list and perhaps gregkh, so I can > > see if > > I can resolve it by rebasing my patches, or if he would like to merge > > the > > patches. > > Pulling the y2038-endgame branch does lead to some conflicts, which are > currently still kinda staightforward to solve. > > However I'd be very interested in getting this branch merged to v5.4, > so we don't run into more difficult merge conflicts the coming years > where the v5.4-LTS still gets stable updates (Dec, 2025) and possibly > to get any related fixes from upstream. > > @Greg: any chance to get the y2038-endgame merged into v5.4.y? I have no idea what this really means, and what it entails, but odds are, no :) Why not just use a newer kernel? Why are you stuck using a 5.4 kernel for a device that has to live in 2038? That feels very foolish to me... thanks, greg k-h
Re: [PATCH RFC v2 02/18] irq/dev-msi: Add support for a new DEV_MSI irq domain
On Fri, Aug 07, 2020 at 09:06:50AM -0300, Jason Gunthorpe wrote: > On Thu, Aug 06, 2020 at 10:21:11PM +0200, Thomas Gleixner wrote: > > > Optionally? Please tell the hardware folks to make this mandatory. We > > have enough pain with non maskable MSI interrupts already so introducing > > yet another non maskable interrupt trainwreck is not an option. > > Can you elaborate on the flows where Linux will need to trigger > masking? > > I expect that masking will be available in our NIC HW too - but it > will require a spin loop if masking has to be done in an atomic > context. > > > It's more than a decade now that I tell HW people not to repeat the > > non-maskable MSI failure, but obviously they still think that > > non-maskable interrupts are a brilliant idea. I know that HW folks > > believe that everything they omit can be fixed in software, but they > > have to finally understand that this particular issue _cannot_ be fixed > > at all. > > Sure, the CPU should always be able to shut off an interrupt! > > Maybe explaining the goals would help understand the HW perspective. > > Today HW can process > 100k queues of work at once. Interrupt delivery > works by having a MSI index in each queue's metadata and the interrupt > indirects through a MSI-X table on-chip which has the > addr/data/mask/etc. > > What IMS proposes is that the interrupt data can move into the queue > meta data (which is not required to be on-chip), eg along side the > producer/consumer pointers, and the central MSI-X table is not > needed. This is necessary because the PCI spec has very harsh design > requirements for a MSI-X table that make scaling it prohibitive. > > So an IRQ can be silenced by deleting or stopping the queue(s) > triggering it. It can be masked by including masking in the queue > metadata. We can detect pending by checking the producer/consumer > values. > > However synchronizing all the HW and all the state is now more > complicated than just writing a mask bit via MMIO to an on-die memory. Because doing all of the work that used to be done in HW in software is so much faster and scalable? Feels really wrong to me :( Do you all have a pointer to the spec for this newly proposed stuff anywhere to try to figure out how the HW wants this to all work? thanks, greg k-h
Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes
On Tue, Aug 04, 2020 at 08:08:10AM +, 吳昊澄 Ricky wrote: > > -Original Message- > > From: gre...@linuxfoundation.org [mailto:gre...@linuxfoundation.org] > > Sent: Tuesday, August 04, 2020 3:49 PM > > To: 吳昊澄 Ricky > > Cc: Chris Clayton; LKML; rdun...@infradead.org; philqua...@gmail.com; Arnd > > Bergmann > > Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader > > on > > Intel NUC boxes > > > > On Tue, Aug 04, 2020 at 02:44:41AM +, 吳昊澄 Ricky wrote: > > > Hi Chris, > > > > > > rtsx_pci_write_register(pcr, FPDTL, OC_POWER_DOWN, OC_POWER_DOWN); > > > This register operation saved power under 1mA, so if do not care the 1mA > > power we can have a patch to remove it, make compatible with NUC6 > > > We tested others our card reader that remove it, we did not see any side > > > effect > > > > > > Hi Greg k-h, > > > > > > Do you have any comments? > > > > comments on what? I don't know what you are responding to here, sorry. > > > Can we have a patch to kernel for NUC6? It may cause more power(1mA) > but it will have more compatibility I do not understand at all, sorry. greg k-h
Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes
On Tue, Aug 04, 2020 at 02:44:41AM +, 吳昊澄 Ricky wrote: > Hi Chris, > > rtsx_pci_write_register(pcr, FPDTL, OC_POWER_DOWN, OC_POWER_DOWN); > This register operation saved power under 1mA, so if do not care the 1mA > power we can have a patch to remove it, make compatible with NUC6 > We tested others our card reader that remove it, we did not see any side > effect > > Hi Greg k-h, > > Do you have any comments? comments on what? I don't know what you are responding to here, sorry. greg k-h
Re: 答复: [PATCH] USB:Fix kernel NULL pointer when unbind UHCI form vfio-pci
On Thu, Jul 23, 2020 at 08:36:25AM +, WeitaoWang-oc wrote:
>
> On Thu,23 July 2020 04:18:00 + Alex wrote:
> > On Wed, 22 Jul 2020 19:57:48 +0800
> > WeitaoWangoc wrote:
> >
> > > drivers/usb/core/hcd-pci.c | 5 +
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
> > > index 1547aa6..484f2a0 100644
> > > --- a/drivers/usb/core/hcd-pci.c
> > > +++ b/drivers/usb/core/hcd-pci.c
> > > @@ -34,6 +34,7 @@ static DECLARE_RWSEM(companions_rwsem);
> > > #define CL_OHCIPCI_CLASS_SERIAL_USB_OHCI
> > > #define CL_EHCIPCI_CLASS_SERIAL_USB_EHCI
> > >
> > > +#define PCI_DEV_DRV_FLAG 2
> > > static inline int is_ohci_or_uhci(struct pci_dev *pdev) {
> > > return pdev->class == CL_OHCI || pdev->class == CL_UHCI; @@
> > > -68,6 +69,8 @@ static void for_each_companion(struct pci_dev *pdev, struct
> > usb_hcd *hcd,
> > > if (companion->class != CL_UHCI && companion->class !=
> > CL_OHCI &&
> > > companion->class != CL_EHCI)
> > > continue;
> > > + if (!(companion->priv_flags & PCI_DEV_DRV_FLAG))
> >
> > But pci_dev.priv_flags is private data for the driver that currently
> > owns the device, which could be vfio-pci. This is really no different
> > than assuming the structure at device.driver_data. If vfio-pci were to
> > make legitimate use of pci_dev.priv_flags, this could simply blow up
> > again. Should there instead be some sort of registration interface
> > where hcd complaint drivers register their devices and only those
> > registered devices can have their driver private data arbitrarily poked
> > by another driver? Thanks,
>
> Thanks for your explanation. Set pci_dev.priv_flags is really not a
> reasonable approach. Are there any more detailed suggestions
> to patch this issue?
This is not a kernel issue, it is a "do not do this in this way from
userspace" issue :)
thanks,
greg k-h
Re: [PATCH 5.7 233/244] RISC-V: Acquire mmap lock before invoking walk_page_range
On Mon, Jul 20, 2020 at 06:50:10PM +, Atish Patra wrote: > On Mon, 2020-07-20 at 23:11 +0530, Naresh Kamboju wrote: > > RISC-V build breaks on stable-rc 5.7 branch. > > build failed with gcc-8, gcc-9 and gcc-9. > > > > Sorry for the compilation issue. > > mmap_read_lock was intrdouced in the following commit. > > commit 9740ca4e95b4 > Author: Michel Lespinasse > Date: Mon Jun 8 21:33:14 2020 -0700 > > mmap locking API: initial implementation as rwsem wrappers > > The following two commits replaced the usage of mmap_sem rwsem calls > with mmap_lock. > > d8ed45c5dcd4 (mmap locking API: use coccinelle to convert mmap_sem > rwsem call sites) > 89154dd5313f (mmap locking API: convert mmap_sem call sites missed by > coccinelle) > > The first commit is not present in stale 5.7-y for obvious reasons. > > Do we need to send a separate patch only for stable branch with > mmap_sem ? I am not sure if that will cause a conflict again in future. I do not like taking odd backports, and would rather take the real patch that is upstream. I will drop this patch from the tree now, so everyone can figure out what they want to do in the future :) thanks, greg k-h
Re: [PATCH RFC 0/5] Introduced new Cadence USBSSP DRD Driver.
On Mon, Jun 29, 2020 at 03:41:49AM +, Peter Chen wrote: > On 20-06-26 07:19:56, Pawel Laszczak wrote: > > Hi Felipe, > > > > > > > >Hi, > > > > > >Pawel Laszczak writes: > > >> This patch introduce new Cadence USBSS DRD driver to linux kernel. > > >> > > >> The Cadence USBSS DRD Controller is a highly configurable IP Core which > > >> can be instantiated as Dual-Role Device (DRD), Peripheral Only and > > >> Host Only (XHCI)configurations. > > >> > > >> The current driver has been validated with FPGA burned. We have support > > >> for PCIe bus, which is used on FPGA prototyping. > > >> > > >> The host side of USBSS-DRD controller is compliance with XHCI > > >> specification, so it works with standard XHCI Linux driver. > > >> > > >> The host side of USBSS DRD controller is compliant with XHCI. > > >> The architecture for device side is almost the same as for host side, > > >> and most of the XHCI specification can be used to understand how > > >> this controller operates. > > >> > > >> This controller and driver support Full Speed, Hight Speed, Supper Speed > > >> and Supper Speed Plus USB protocol. > > >> > > >> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver. > > >> The last letter of this acronym means PLUS. The formal name of controller > > >> is USBSSP but it's to generic so I've decided to use CDNSP. > > >> > > >> The patch 1: adds DT binding. > > >> The patch 2: adds PCI to platform wrapper used on Cadnece testing > > >> platform. It is FPGA based on platform. > > >> The patches 3-5: add the main part of driver and has been intentionally > > >> split into 3 part. In my opinion such division should not > > >> affect understanding and reviewing the driver, and cause > > >> that > > >> main patch (4/5) is little smaller. Patch 3 introduces main > > >> header file for driver, 4 is the main part that implements > > >> all > > >> functionality of driver and 5 introduces tracepoints. > > > > > >I'm more interested in how is this different from CDNS3. Aren't they SW > > >compatible? > > > > In general, the controller can be split into 2 part- DRD part and the rest > > UDC. > > > > The second part UDC which consist gadget.c, ring.c and mem.c file is > > completely different. > > > > The DRD part contains drd.c and core.c. > > cdnsp drd.c is similar to cdns3 drd.c but it's little different. CDNSP has > > similar, but has different register space. > > Some register was moved, some was removed and some was added. > > > > core.c is very similar and eventually could be common for both drivers. I > > thought about this but > > I wanted to avoid interfering with cdns3 driver at this point CDNSP is > > still under testing and > > CDNS3 is used by some products on the market. > > Pawel, I suggest adding CDNSP at driver/staging first since it is still > under testing. When you are thinking the driver (as well as hardware) are > mature, you could try to add gadget part (eg, gadget-v2) and make > necessary changes for core.c. I only take code for drivers/staging/ that for some reason is not meeting the normal coding style/rules/whatever. For stuff that is an obvious duplicate of existing code like this, and needs to be rearchitected. It is much more work to try to convert code once it is in the tree than to just do it out of the tree on your own and resubmit it, as you don't have to follow the in-kernel rules of "one patch does one thing" that you would if it was in staging. So don't think that staging is the right place for this, just spend a few weeks to get it right and then resubmit it. thanks, greg k-h
Re: [PATCH RFC 0/5] Introduced new Cadence USBSSP DRD Driver.
On Mon, Jun 29, 2020 at 11:20:00AM +, Pawel Laszczak wrote: > > >> > Hi Felipe, > >> > > >> > > > >> > >Hi, > >> > > > >> > >Pawel Laszczak writes: > >> > >> This patch introduce new Cadence USBSS DRD driver to linux kernel. > >> > >> > >> > >> The Cadence USBSS DRD Controller is a highly configurable IP Core > >> > >> which > >> > >> can be instantiated as Dual-Role Device (DRD), Peripheral Only and > >> > >> Host Only (XHCI)configurations. > >> > >> > >> > >> The current driver has been validated with FPGA burned. We have > >> > >> support > >> > >> for PCIe bus, which is used on FPGA prototyping. > >> > >> > >> > >> The host side of USBSS-DRD controller is compliance with XHCI > >> > >> specification, so it works with standard XHCI Linux driver. > >> > >> > >> > >> The host side of USBSS DRD controller is compliant with XHCI. > >> > >> The architecture for device side is almost the same as for host side, > >> > >> and most of the XHCI specification can be used to understand how > >> > >> this controller operates. > >> > >> > >> > >> This controller and driver support Full Speed, Hight Speed, Supper > >> > >> Speed > >> > >> and Supper Speed Plus USB protocol. > >> > >> > >> > >> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver. > >> > >> The last letter of this acronym means PLUS. The formal name of > >> > >> controller > >> > >> is USBSSP but it's to generic so I've decided to use CDNSP. > >> > >> > >> > >> The patch 1: adds DT binding. > >> > >> The patch 2: adds PCI to platform wrapper used on Cadnece testing > >> > >> platform. It is FPGA based on platform. > >> > >> The patches 3-5: add the main part of driver and has been > >> > >> intentionally > >> > >> split into 3 part. In my opinion such division should not > >> > >> affect understanding and reviewing the driver, and cause > >> > >> that > >> > >> main patch (4/5) is little smaller. Patch 3 introduces > >> > >> main > >> > >> header file for driver, 4 is the main part that > >> > >> implements all > >> > >> functionality of driver and 5 introduces tracepoints. > >> > > > >> > >I'm more interested in how is this different from CDNS3. Aren't they SW > >> > >compatible? > >> > > >> > In general, the controller can be split into 2 part- DRD part and the > >> > rest UDC. > >> > > >> > The second part UDC which consist gadget.c, ring.c and mem.c file is > >> > completely different. > >> > > >> > The DRD part contains drd.c and core.c. > >> > cdnsp drd.c is similar to cdns3 drd.c but it's little different. CDNSP > >> > has similar, but has different register space. > >> > Some register was moved, some was removed and some was added. > >> > > >> > core.c is very similar and eventually could be common for both drivers. > >> > I thought about this but > >> > I wanted to avoid interfering with cdns3 driver at this point CDNSP is > >> > still under testing and > >> > CDNS3 is used by some products on the market. > >> > >> Pawel, I suggest adding CDNSP at driver/staging first since it is still > >> under testing. When you are thinking the driver (as well as hardware) are > >> mature, you could try to add gadget part (eg, gadget-v2) and make > >> necessary changes for core.c. > > > >I only take code for drivers/staging/ that for some reason is not > >meeting the normal coding style/rules/whatever. For stuff that is an > >obvious duplicate of existing code like this, and needs to be > >rearchitected. It is much more work to try to convert code once it is > >in the tree than to just do it out of the tree on your own and resubmit > >it, as you don't have to follow the in-kernel rules of "one patch does > >one thing" that you would if it was in staging. > > > >So don't think that staging is the right place for this, just spend a > >few weeks to get it right and then resubmit it. > > > > Ok, > I try to reuse the code from cdns3. Where such common code should be > placed ? Should I move it to e.g. drivers/usb/common/cdns or it should remain > in > cdns3 directory. In the cdns3 directory makes the most sense. thanks, greg k-h
Re: [PATCH v3 2/2] USB: serial: cp210x: Proper RTS control when buffers fill
On Wed, Jun 24, 2020 at 07:03:04AM +, Phu Luu wrote:
> CP210x hardware disables auto-RTS but leaves auto-CTS when
> in hardware flow control mode and UART on cp210x hardware
> is disabled. This allows data to flow out, but new data
> will not come into the port. When re-opening the port, if
> auto-CTS is enabled on the cp210x, then auto-RTS must be
> re-enabled in the driver.
>
> Signed-off-by: Phu Luu
> Signed-off-by: Brant Merryman
> ---
> 06/09/2020: Patch v3 2/2 Modified based on feedback from Johan Hovold
>
> 12/18/2019: Patch v2 Broken into two patches and modified based on feedback
> from Johan Hovold
> 12/09/2019: Initial submission of patch "Proper RTS control when buffers fill"
>
> drivers/usb/serial/cp210x.c | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index bcceb4ad8be0..091441b1c5b9 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -917,6 +917,7 @@ static void cp210x_get_termios_port(struct
> usb_serial_port *port,
>u32 baud;
>u16 bits;
>u32 ctl_hs;
> + u32 flow_repl;
> cp210x_read_u32_reg(port, CP210X_GET_BAUDRATE, );
> @@ -1017,6 +1018,23 @@ static void cp210x_get_termios_port(struct
> usb_serial_port *port,
>ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake);
>if (ctl_hs & CP210X_SERIAL_CTS_HANDSHAKE) {
>dev_dbg(dev, "%s - flow control = CRTSCTS\n",
> __func__);
> + /*
> + * When the port is closed, the CP210x hardware
> disables
> + * auto-RTS and RTS is deasserted but it leaves
> auto-CTS when
> + * in hardware flow control mode. This prevents
> new data from
> + * being received by the port. When re-opening
> the port, if
> + * auto-CTS is enabled on the cp210x, then
> auto-RTS must be
> + * re-enabled in the driver.
> + */
> + flow_repl = le32_to_cpu(flow_ctl.ulFlowReplace);
> + flow_repl &= ~CP210X_SERIAL_RTS_MASK;
> + flow_repl |=
> CP210X_SERIAL_RTS_SHIFT(CP210X_SERIAL_RTS_FLOW_CTL);
> + flow_ctl.ulFlowReplace = cpu_to_le32(flow_repl);
> + cp210x_write_reg_block(port,
> + CP210X_SET_FLOW,
> + _ctl,
> +
> sizeof(flow_ctl));
> +
>cflag |= CRTSCTS;
>} else {
>dev_dbg(dev, "%s - flow control = NONE\n",
> __func__);
> --
> 2.17.0
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- Your patch is malformed (tabs converted to spaces, linewrapped, etc.)
and can not be applied. Please read the file,
Documentation/email-clients.txt in order to fix this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
Re: Subject: [PATCH v3 1/2] USB: serial: cp210x: Enable usb generic throttle/unthrottle
On Wed, Jun 24, 2020 at 04:11:21AM +, Phu Luu wrote: > Assign the .throttle and .unthrottle functions to be generic function > in the driver structure to prevent data loss that can otherwise occur > if the host does not enable USB throttling. > > Signed-off-by: Phu Luu An > Signed-off-by: Brant Merryman Why does your Subject: line have "Subject:" in it again? Can you please fix up and resend these patches? thanks, greg k-h
Re: [PATCH] USB: Serial: option: add a Longcheer device id
On Tue, Jun 16, 2020 at 07:55:10AM +, Gao, Nian wrote:
> >From 9220e0828f70a34c968e32d34e13281a4ff54b08 Mon Sep 17 00:00:00 2001
> From: Gao Nian
> Date: Tue, 16 Jun 2020 15:13:48 +0800
> Subject: [PATCH] USB: Serial: option: add a Longcheer device id
>
> Add a Longcheer device-id entry which specifically enables
> U9300C TD-LTE module.
>
> Signed-off-by: Gao Nian
> ---
> drivers/usb/serial/option.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
> index 254a8bbeea67..6fd92431d2c9 100644
> --- a/drivers/usb/serial/option.c
> +++ b/drivers/usb/serial/option.c
> @@ -394,6 +394,9 @@ static void option_instat_callback(struct urb *urb);
> /* iBall 3.5G connect wireless modem */
> #define IBALL_3_5G_CONNECT 0x9605
> +/* LONGSUNG U9300C TD-LTE wireless modem */
> +#define LONGSUNG_U9300C 0x9b3c
> +
> /* Zoom */
> #define ZOOM_PRODUCT_4597 0x9607
> @@ -1845,6 +1848,7 @@ static const struct usb_device_id option_ids[] = {
> .driver_info = RSVD(4) },
>{ USB_DEVICE(LONGCHEER_VENDOR_ID, ZOOM_PRODUCT_4597) },
>{ USB_DEVICE(LONGCHEER_VENDOR_ID, IBALL_3_5G_CONNECT) },
> + { USB_DEVICE(LONGCHEER_VENDOR_ID, LONGSUNG_U9300C) },
>{ USB_DEVICE(HAIER_VENDOR_ID, HAIER_PRODUCT_CE100) },
>{ USB_DEVICE_AND_INTERFACE_INFO(HAIER_VENDOR_ID,
> HAIER_PRODUCT_CE81B, 0xff, 0xff, 0xff) },
>/* Pirelli */
> --
> 2.25.1
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- Your patch is malformed (tabs converted to spaces, linewrapped, etc.)
and can not be applied. Please read the file,
Documentation/email-clients.txt in order to fix this.
- Your patch was attached, please place it inline so that it can be
applied directly from the email message itself.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
Re: [PATCH] drivers: fix the hardware flow function of cp2102
On Tue, Jun 16, 2020 at 08:37:39AM +, Gao, Nian wrote:
> >From 97278cc3d00d22e8fc1edecce1f08772823a50dd Mon Sep 17 00:00:00 2001
> From: Gao Nian
> Date: Tue, 16 Jun 2020 16:29:42 +0800
> Subject: [PATCH] drivers: fix the hardware flow function of cp2102
Why is this all in the body of your email?
Please just use git send-email to send patches out so that they come in
the proper format.
As it is, your patch has all of the tabs changed to spaces, making it
impossible to apply. Please fix your email client up to not do this.
>
> When the recieve buffer is full in hardware flow mode,
> cp2102 will not activate the RTS signal to notify
> the sender to stop sending data.
>
> Signed-off-by: Gao Nian
> ---
> drivers/usb/serial/cp210x.c | 17 -
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index f5143eedbc48..c3e05e135d2d 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -272,6 +272,8 @@ static struct usb_serial_driver cp210x_device = {
>.break_ctl = cp210x_break_ctl,
>.set_termios = cp210x_set_termios,
>.tx_empty = cp210x_tx_empty,
> + .throttle = usb_serial_generic_throttle,
> + .unthrottle = usb_serial_generic_unthrottle,
>.tiocmget = cp210x_tiocmget,
>.tiocmset = cp210x_tiocmset,
>.attach = cp210x_attach,
> @@ -915,6 +917,7 @@ static void cp210x_get_termios_port(struct
> usb_serial_port *port,
>u32 baud;
>u16 bits;
>u32 ctl_hs;
> + u32 flow_repl;
> cp210x_read_u32_reg(port, CP210X_GET_BAUDRATE, );
> @@ -1013,8 +1016,20 @@ static void cp210x_get_termios_port(struct
> usb_serial_port *port,
>cp210x_read_reg_block(port, CP210X_GET_FLOW, _ctl,
>sizeof(flow_ctl));
>ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake);
> + flow_repl = le32_to_cpu(flow_ctl.ulFlowReplace);
> + /*
> + * CP210x hardware disables RTS but leaves CTS when in hardware flow
> + * control mode and port is closed.
> + * This allows data to flow out, but new data will not come into
> the port.
> + * When re-opening the port, if CTS is enabled, then RTS must
> manually be
> + * re-enabled.
> + */
>if (ctl_hs & CP210X_SERIAL_CTS_HANDSHAKE) {
> - dev_dbg(dev, "%s - flow control = CRTSCTS\n",
> __func__);
Why remove this debugging line?
thanks,
greg k-h
Re: [RFC PATCH 1/3] Documentation/x86: Add documentation for /proc/cpuinfo feature flags
On Mon, Jun 15, 2020 at 06:31:50PM +, Luck, Tony wrote: > > In general, this should say something along the lines that /proc/cpuinfo > > shows features which the kernel supports. > > > > "For a full list of CPUID flags which the CPU supports, use > > tools/arch/x86/tools/cpuid/cpuid" > > > > :-) > > Dave Hansen had suggested (offline) that we add a cpuid tool to the kernel > sources. > > I think that's a lot of (duplicated) work for someone to maintain. The > version of this > tool at http://www.etallen.com/cpuid.html is close to 10K lines of code. 10k is nothing, have you looked at tools/perf/ ? :) greg k-h
Re: CMA enhancement - non-default areas in x86
On Wed, May 13, 2020 at 09:43:45AM +, Ravich, Leonid wrote: > > On Wed, May 13, 2020 at 08:29:16AM +, Ravich, Leonid wrote: > > > PCIe NTB > > > Documentation/driver-api/ntb.rst > > > > > 1) Basically PCI bridge between to root complex / PCI switches > > > 2) using out of OS memory is one solution but then this memory is > > > Limited for usage by other stack, ex: get_user_pages on this memory > > > will fail, Therefore attempting to use it for block layer with (o_direct) > > > will > > fail. > > > > > > Acutely any generic stack which attempts to "pin" this memory will fail. > > > > So why isn't the BIOS/UEFI properly reserving this from the general > > operating > > system's pages so that the driver knows to use them instead? > > > > Is UEFI wrong here about these being valid memory ranges for general use? > > If so, why not fix that? If not, how in the world is the OS supposed to > > know > > these memory ranges are _not_ for general use? I feel like there is > > something missing here... > > > Maybe I am miss understanding something here , but if BIOS/UEFI will reserve > this pages > They will be "out of kernel" which will work for propriety driver but this > memory will not > be useable for generic driver which will attempt to pin this memory with > get_user_pages() . > so we can go and try to fix that (not sure this is the right way) . What do you mean by "propriety" driver vs. "generic" driver? Shouldn't there be some "generic" way that UEFI tells any driver where these memory locations are that can not be used as general memory? If not, try fixing up UEFI for that. > another option here is to use some kernel infrastructure which from one > side reserve the memory from general use > on the other hand kernel will be aware of this pages so get_user_pages() > will work on this memory . > > from what we saw CMA infrastructure can support such requirements. CMA needs to be told where to reserve the memory at boot time. If you want to use that, great, but something has to tell it, so perhaps just get that info from UEFI as that is the "equilivant" to a device tree, right? Try it all out and see, all of this is pointless without real patches, which is why we almost never have these kinds of discussions without working code. thanks, greg k-h
Re: CMA enhancement - non-default areas in x86
A: http://en.wikipedia.org/wiki/Top_post Q: Were do I find info about this thing called top-posting? A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? A: No. Q: Should I include quotations after my reply? http://daringfireball.net/2007/07/on_top On Wed, May 13, 2020 at 08:29:16AM +, Ravich, Leonid wrote: > PCIe NTB > Documentation/driver-api/ntb.rst > 1) Basically PCI bridge between to root complex / PCI switches > 2) using out of OS memory is one solution but then this memory is > Limited for usage by other stack, ex: get_user_pages on this memory will > fail, > Therefore attempting to use it for block layer with (o_direct) will fail. > > Acutely any generic stack which attempts to "pin" this memory will fail. So why isn't the BIOS/UEFI properly reserving this from the general operating system's pages so that the driver knows to use them instead? Is UEFI wrong here about these being valid memory ranges for general use? If so, why not fix that? If not, how in the world is the OS supposed to know these memory ranges are _not_ for general use? I feel like there is something missing here... thanks, greg k-h
Re: CMA enhancement - non-default areas in x86
On Wed, May 13, 2020 at 07:00:12AM +, Idgar, Or wrote: > > For what type of device? > NTB (Non-Transparent Bridge). Very odd quoting style... Anyway, what exactly is a non-transparent bridge, and why doesn't your bios/uefi implementation properly reserve the memory for it so that the OS does not use it? thanks, greg k-h
Re: CMA enhancement - non-default areas in x86
On Wed, May 13, 2020 at 06:13:55AM +, Idgar, Or wrote: > Hi, > I'm working with Linux kernel on x86 and needed a way to allocate a very > large contiguous memory (around 20GB) for DMA operations. For what type of device? > I've found out that CMA is one of the major ways to do so, but our problem is > that CMA's default behavior is to create one default area from which all > devices can allocate memory. > when booting, there were some drivers that allocated memory for DMA and used > CMA memory if exist. The problem is that it takes memory that we need for our > device and we want to make sure this area is dedicated for our device. > > As I saw, the only way to reserve a dedicated area is by enabling > OF_RESERVED_MEM which is available for several architectures but excluding > x86 (and as far as I understand relies on device tree which is not in use > with x86 or at least cannot be configured with OF_RESERVED_MEM). > > I really want to leverage this mechanism/API and thought about modifying the > code (and hopefully merge it upstream) so multiple non-default areas will be > available for x86 and with a way to consume it by mapping specific area to > specific device. > > Is it something that will be open for merging if written properly? We always will be glad to review patches, no need to ask us about that. Just post them! good luck, greg k-h
Re: [PATCH 0/1] Add IMR driver for Keem Bay
On Fri, May 01, 2020 at 08:53:34AM +0100, Daniele Alessandrelli wrote: > On Fri, 2020-05-01 at 09:09 +0200, gre...@linuxfoundation.org wrote: > > On Thu, Apr 30, 2020 at 07:49:36PM +, Alessandrelli, Daniele > > wrote: > > > This e-mail and any attachments may contain confidential material > > > for the sole > > > use of the intended recipient(s). Any review or distribution by > > > others is > > > strictly prohibited. If you are not the intended recipient, please > > > contact the > > > sender and delete all copies. > > > > This footer means I delete all of your emails... > > My bad, I replied using the wrong email address (i.e., the one that > adds the footer automatically). Sorry about that :/ > I'll be more careful the next time. > > The original emails (the ones with the cover letter and the patch) were > fine though (unless I did something else wrong). Any advice on how to > have the patch reviewed and eventually merged? > > > > > > > Ok, let me go look at the code...
Re: [PATCH 0/1] Add IMR driver for Keem Bay
On Thu, Apr 30, 2020 at 07:49:36PM +, Alessandrelli, Daniele wrote: > This e-mail and any attachments may contain confidential material for the sole > use of the intended recipient(s). Any review or distribution by others is > strictly prohibited. If you are not the intended recipient, please contact the > sender and delete all copies. This footer means I delete all of your emails...
Re: [PATCH v4] Add new uio device for PCI with dynamic memory allocation
On Wed, Apr 29, 2020 at 07:51:01AM +, Stahl, Manuel wrote: > On Di, 2020-04-28 at 15:54 +0200, gregkh @ linuxfoundation . org wrote: > > On Thu, Apr 16, 2020 at 06:38:30PM +0200, Manuel Stahl wrote: > > > > > > + * > > > + * Since the driver does not declare any device ids, you must allocate > > > + * id and bind the device to the driver yourself. For example: > > > + * > > > + * # echo "8086 10f5" > /sys/bus/pci/drivers/uio_pci_dmem_genirq/new_id > > > + * # echo -n :00:19.0 > /sys/bus/pci/drivers/e1000e/unbind > > > + * # echo -n :00:19.0 > /sys/bus/pci/drivers/uio_pci_dmem_genirq/bind > > > + * # ls -l /sys/bus/pci/devices/:00:19.0/driver > > > + * .../:00:19.0/driver -> > > > ../../../bus/pci/drivers/uio_pci_dmem_genirq > > > + * > > > + * Or use a modprobe alias: > > > + * # alias pci:v10EEd1000sv*sd*sc*i* uio_pci_dmem_genirq > > > + * > > > + * Driver won't bind to devices which do not support the Interrupt > > > Disable Bit > > > + * in the command register. All devices compliant to PCI 2.3 (circa > > > 2002) and > > > + * all compliant PCI Express devices should support this bit. > > > + * > > > + * The DMA mask bits and sizes of dynamic regions are derived from module > > > + * parameters. > > > + * > > > + * The format for specifying dynamic region sizes in module parameters > > > + * is as follows: > > > + * > > > + * uio_pci_dmem_genirq.dmem_sizes := > > > [;] > > > + *:= :[,] > > > + *:= : > > > + * := standard linux memsize > > > + * > > > + * Examples: > > > + * > > > + * 1) UIO dmem device with 3 dynamic regions: > > > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M > > > + * > > > + * 2) Two UIO dmem devices with different number of dynamic regions: > > > + * uio_pci_dmem_genirq.dmem_sizes=8086:10f5:4K,16K,4M;1234:0001:8K > > > > Module parameters are horrid, are you sure there is no other way? > > You're right, seemed to be the simplest solution back when we started > developing this driver. I will try to change it to sysfs, so that one can add > regions while the module is already loaded. /me hands you some \n characters... Anyway, configfs is for configuring stuff, don't make a sysfs file that you have to somehow "parse" please. thanks, greg k-h
Re: [PATCH v4] Add new uio device for PCI with dynamic memory allocation
On Tue, Apr 28, 2020 at 03:47:42PM +, Stahl, Manuel wrote: > > > > > + return err; > > > + } > > > + pci_set_master(pdev); > > > + > > > + dev_info(>dev, "Legacy IRQ: %i", pdev->irq); > > > > Again, remove, be quiet :) > > Use dev_dbg() or remove completely? If it helps in debugging, dev_dbg() is fine to use. thanks, greg k-h
Re: [PATCH RESEND v2] staging: wfx: fix an undefined reference error when CONFIG_MMC=m
On Mon, Oct 14, 2019 at 10:06:25AM +, Jerome Pouiller wrote: > On Monday 14 October 2019 11:53:19 CEST Jérôme Pouiller wrote: > [...] > > Hello Zhong, > > > > Now, I see the problem. It happens when CONFIG_MMC=m and CONFIG_WFX=y > > (if CONFIG_WFX=m, it works). > > > > I think the easiest way to solve problem is to disallow CONFIG_WFX=y if > > CONFIG_MMC=m. > > > > This solution impacts users who want to use SPI bus with configuration: > > CONFIG_WFX=y + CONFIG_SPI=y + CONFIG_MMC=m. However, I think this is a > > twisted case. So, I think it won't be missed. > > > > I think that patch below do the right thing: > > > > -8<--8<--8<- > > > > diff --git i/drivers/staging/wfx/Kconfig w/drivers/staging/wfx/Kconfig > > index 9b8a1c7a9e90..833f3b05b6b4 100644 > > --- i/drivers/staging/wfx/Kconfig > > +++ w/drivers/staging/wfx/Kconfig > > @@ -1,7 +1,7 @@ > > config WFX > > tristate "Silicon Labs wireless chips WF200 and further" > > depends on MAC80211 > > - depends on (SPI || MMC) > > + depends on (MMC=m && m) || MMC=y || (SPI && MMC!=m) > > help > > This is a driver for Silicons Labs WFxxx series (WF200 and > > further) > > chipsets. This chip can be found on SPI or SDIO buses. > > > > > > > > An alternative (more understandable?): > > diff --git i/drivers/staging/wfx/Kconfig w/drivers/staging/wfx/Kconfig > index 9b8a1c7a9e90..83ee4d0ca8c6 100644 > --- i/drivers/staging/wfx/Kconfig > +++ w/drivers/staging/wfx/Kconfig > @@ -1,6 +1,7 @@ > config WFX > tristate "Silicon Labs wireless chips WF200 and further" > depends on MAC80211 > + depends on MMC || !MMC # do not allow WFX=y if MMC=m > depends on (SPI || MMC) > help > This is a driver for Silicons Labs WFxxx series (WF200 and further) Yes, this is much easier to understand. thanks, greg k-h
Re: [PATCH v2] kheaders: making headers archive reproducible
On Fri, Oct 04, 2019 at 10:40:07AM +, Dmitry Goldin wrote:
> From: Dmitry Goldin
>
> In commit 43d8ce9d65a5 ("Provide in-kernel headers to make
> extending kernel easier") a new mechanism was introduced, for kernels
> >=5.2, which embeds the kernel headers in the kernel image or a module
> and exposes them in procfs for use by userland tools.
>
> The archive containing the header files has nondeterminism caused by
> header files metadata. This patch normalizes the metadata and utilizes
> KBUILD_BUILD_TIMESTAMP if provided and otherwise falls back to the
> default behaviour.
>
> In commit f7b101d33046 ("kheaders: Move from proc to sysfs") it was
> modified to use sysfs and the script for generation of the archive was
> renamed to what is being patched.
>
> Signed-off-by: Dmitry Goldin
Reviewed-by: Greg Kroah-Hartman
Re: [PATCH] async: Let kfree() out of the critical area of the lock
On Wed, Sep 25, 2019 at 08:52:26PM +0800, Yunfeng Ye wrote: > It's not necessary to put kfree() in the critical area of the lock, so > let it out. > > Signed-off-by: Yunfeng Ye > --- > kernel/async.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/kernel/async.c b/kernel/async.c > index 4f9c1d6..1de270d 100644 > --- a/kernel/async.c > +++ b/kernel/async.c > @@ -135,12 +135,12 @@ static void async_run_entry_fn(struct work_struct *work) > list_del_init(>domain_list); > list_del_init(>global_list); > > - /* 3) free the entry */ > - kfree(entry); > atomic_dec(_count); > - > spin_unlock_irqrestore(_lock, flags); > > + /* 3) free the entry */ > + kfree(entry); > + > /* 4) wake up any waiters */ > wake_up(_done); > } > -- > 2.7.4 > Does this result any any measurable performance changes? thanks, greg k-h
Re: [PATCH 4/4] dmaengine: Add debugfs entries for PTDMA information
On Tue, Sep 24, 2019 at 07:33:02AM +, Mehta, Sanju wrote:
> +static const struct file_operations pt_debugfs_info_ops = {
> + .owner = THIS_MODULE,
> + .open = simple_open,
> + .read = ptdma_debugfs_info_read,
> + .write = NULL,
> +};
> +
> +static const struct file_operations pt_debugfs_queue_ops = {
> + .owner = THIS_MODULE,
> + .open = simple_open,
> + .read = ptdma_debugfs_queue_read,
> + .write = ptdma_debugfs_queue_write,
> +};
> +
> +static const struct file_operations pt_debugfs_stats_ops = {
> + .owner = THIS_MODULE,
> + .open = simple_open,
> + .read = ptdma_debugfs_stats_read,
> + .write = ptdma_debugfs_stats_write,
> +};
Can you use DEFINE_SIMPLE_ATTRIBUTE() here intead of these?
thanks,
greg k-h
Re: [PATCH v3 4/7] serial: fsl_linflexuart: Be consistent with the name
On Fri, Aug 23, 2019 at 07:11:37PM +, Stefan-gabriel Mirea wrote: > For consistency reasons, spell the controller name as "LINFlexD" in > comments and documentation. > > Signed-off-by: Stefan-Gabriel Mirea > --- > Documentation/admin-guide/kernel-parameters.txt | 2 +- > drivers/tty/serial/Kconfig | 8 > drivers/tty/serial/fsl_linflexuart.c| 4 ++-- > include/uapi/linux/serial_core.h| 4 ++-- > 4 files changed, 9 insertions(+), 9 deletions(-) Doesn't apply to my tty-next tree :(
Re: [PATCH 5/6] tty: serial: Add linflexuart driver for S32V234
On Tue, Aug 06, 2019 at 05:11:17PM +, Stefan-gabriel Mirea wrote: > On 8/5/2019 6:31 PM, gre...@linuxfoundation.org wrote: > > On Fri, Aug 02, 2019 at 07:47:23PM +, Stefan-gabriel Mirea wrote: > >> > >> +/* Freescale Linflex UART */ > >> +#define PORT_LINFLEXUART 121 > > > > Do you really need this modified? > > Hello Greg, > > This macro is meant to be assigned to port->type in the config_port > method from uart_ops, in order for verify_port to know if the received > serial_struct structure was really targeted for a LINFlex port. It > needs to be defined outside, to avoid "collisions" with other drivers. Yes, I know what it goes to, but does anyone in userspace actually use it? > As far as I see, uart_set_info() will actually fail at the > "baud_base < 9600" check[1], right after calling verify_port(), when > performing an ioctl() on "/dev/console" with TIOCSSERIAL using a > serial_struct obtained with TIOCGSERIAL. This happens because this > reduced version of the LINFlex UART driver will not touch the uartclk > field of the uart_port (as there is currently no clock support). > Therefore, the linflex_config/verify_port() functions, along with the > PORT_LINFLEXUART macro, may be indeed unnecessary at this point (and > should be added later). Is this what you mean? No, see below. > Other than that, I do not see anything wrong with the addition of a > define in serial_core.h for this purpose (which is also what most of the > serial drivers do, including amba-pl011.c, mentioned in > Documentation/driver-api/serial/driver.rst as providing the reference > implementation), so please be more specific. I am getting tired of dealing with merge issues with that list, and no one seems to be able to find where they are really needed for userspace, especially for new devices. What happens if you do not have use it? thanks, greg k-h
Re: [PATCH 1/1] genirq: Properly pair kobject_del with kobject_add
On Fri, Aug 02, 2019 at 08:19:37PM +, Michael Kelley wrote:
> From: gre...@linuxfoundation.org Sent: Thursday,
> August 1, 2019 11:34 PM
> > On Thu, Aug 01, 2019 at 11:53:53PM +, Michael Kelley wrote:
> > > If alloc_descs fails before irq_sysfs_init has run, free_desc in the
> > > cleanup path will call kobject_del even though the kobject has not
> > > been added with kobject_add. Fix this by making the call to
> > > kobject_del conditional on whether irq_sysfs_init has run.
> > >
> > > This problem surfaced because commit aa30f47cf666
> > > ("kobject: Add support for default attribute groups to kobj_type")
> > > makes kobject_del stricter about pairing with kobject_add. If the
> > > pairing is incorrrect, a WARNING and backtrace occur in
> > > sysfs_remove_group because there is no parent.
> > >
> > > Fixes: ecb3f394c5db ("genirq: Expose interrupt information through sysfs")
> > > Signed-off-by: Michael Kelley
> > > ---
> > > kernel/irq/irqdesc.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> > > index 9484e88..5447760 100644
> > > --- a/kernel/irq/irqdesc.c
> > > +++ b/kernel/irq/irqdesc.c
> > > @@ -438,7 +438,8 @@ static void free_desc(unsigned int irq)
> > >* The sysfs entry must be serialized against a concurrent
> > >* irq_sysfs_init() as well.
> > >*/
> > > - kobject_del(>kobj);
> > > + if (irq_kobj_base)
> > > + kobject_del(>kobj);
> >
> > But now you leak the memory of desc as there is no chance it could be
> > freed, because the kobject release function is never called :(
>
> In the alloc_descs error path, when irq_kobj_base is still NULL, the
> kobject code sequence is:
> kobject_init() [as called by alloc_desc]
> kobject_put() [as called by delayed_free_desc]
>
> So I don't think anything leaks.
>
> If irq_kobj_base is not NULL, the kobject code sequence is:
> kobject_init() [as called by alloc_desc]
> kobject_add() [as called by irq_sysfs_add]
> kobject_del() [as called by free_desc]
> kobject_put() [as called by delayed_free_desc]
>
> Again, everything is paired up properly.
>
> >
> > Relying on irq_kobj_base to be present or not seems like an odd test
> > here.
> >
>
> It's the same test that is used in irq_sysfs_add to decide whether to
> call kobject_add. So it makes everything paired up and symmetrical.
Ugh, that's a tangled mess and totally not obvious at all. I'm sure
there's a good reason for all of that, and I really don't want to know
:)
Anyway, yes, you are right, the patch is fine, sorry for the noise.
Acked-by: Greg Kroah-Hartman
greg k-h
Re: [PATCH 4.14 43/43] tipc: pass tunnel dev as NULL to udp_tunnel(6)_xmit_skb
On Sat, Aug 03, 2019 at 08:45:03AM +0800, Xin Long wrote: > On Fri, Aug 2, 2019 at 7:03 PM Rantala, Tommi T. (Nokia - FI/Espoo) > wrote: > > > > On Fri, 2019-08-02 at 09:28 +0200, gre...@linuxfoundation.org wrote: > > > On Thu, Aug 01, 2019 at 10:17:30AM +, Rantala, Tommi T. (Nokia - > > > FI/Espoo) wrote: > > > > Hi, > > > > > > > > This tipc patch added in 4.14.132 is triggering a crash for me, > > > > revert > > > > fixes it. > > > > > > > > Anyone have ideas if some other commits missing in 4.14.x to make > > > > this > > > > work...? > > > > > > Do you also hav a problem with 4.19.y? How about 5.2.y? If not, can > > > you do 'git bisect' to find the patch that fixes the issue? > > > > > > thanks, > > > > > > greg k-h > > > > Hi, please pick this to 4.14.y and 4.19.y, tested that it fixes the > > crash in both: > > > > commit 5684abf7020dfc5f0b6ba1d68eda3663871fce52 > > Author: Xin Long > > Date: Mon Jun 17 21:34:13 2019 +0800 > > > > ip_tunnel: allow not to count pkts on tstats by setting skb's dev > > to NULL > Thanks Rantala, > > sorry for late, I was in a trip. > > The patch belongs to a patchset: > > https://www.spinics.net/lists/netdev/msg578674.html > > So this commit should also be included: > > commit 6f6a8622057c92408930c31698394fae1557b188 > Author: Xin Long > Date: Mon Jun 17 21:34:14 2019 +0800 > > ip6_tunnel: allow not to count pkts on tstats by passing dev as NULL This commit is also included in the following kernel releases: 4.9.186 4.14.134 4.19.59 5.1.18 5.2 so this should all be taken care of, right? If not, please let me know. thanks, greg k-h
Re: [PATCH 4.14 43/43] tipc: pass tunnel dev as NULL to udp_tunnel(6)_xmit_skb
On Fri, Aug 02, 2019 at 11:03:15AM +, Rantala, Tommi T. (Nokia - FI/Espoo) wrote: > On Fri, 2019-08-02 at 09:28 +0200, gre...@linuxfoundation.org wrote: > > On Thu, Aug 01, 2019 at 10:17:30AM +, Rantala, Tommi T. (Nokia - > > FI/Espoo) wrote: > > > Hi, > > > > > > This tipc patch added in 4.14.132 is triggering a crash for me, > > > revert > > > fixes it. > > > > > > Anyone have ideas if some other commits missing in 4.14.x to make > > > this > > > work...? > > > > Do you also hav a problem with 4.19.y? How about 5.2.y? If not, can > > you do 'git bisect' to find the patch that fixes the issue? > > > > thanks, > > > > greg k-h > > Hi, please pick this to 4.14.y and 4.19.y, tested that it fixes the > crash in both: > > commit 5684abf7020dfc5f0b6ba1d68eda3663871fce52 > Author: Xin Long > Date: Mon Jun 17 21:34:13 2019 +0800 > > ip_tunnel: allow not to count pkts on tstats by setting skb's dev > to NULL > > > For 5.2.y nothing is needed, these commits were in v5.2-rc6 already. Thanks for the info, now snuck into the latest round of -rc releases. greg k-h
Re: [PATCH 4.14 43/43] tipc: pass tunnel dev as NULL to udp_tunnel(6)_xmit_skb
On Thu, Aug 01, 2019 at 10:17:30AM +, Rantala, Tommi T. (Nokia - FI/Espoo) wrote: > On Tue, 2019-07-02 at 10:02 +0200, Greg Kroah-Hartman wrote: > > From: Xin Long > > > > commit c3bcde026684c62d7a2b6f626dc7cf763833875c upstream. > > > > udp_tunnel(6)_xmit_skb() called by tipc_udp_xmit() expects a tunnel > > device > > to count packets on dev->tstats, a perpcu variable. However, TIPC is > > using > > udp tunnel with no tunnel device, and pass the lower dev, like veth > > device > > that only initializes dev->lstats(a perpcu variable) when creating > > it. > > Hi, > > This tipc patch added in 4.14.132 is triggering a crash for me, revert > fixes it. > > Anyone have ideas if some other commits missing in 4.14.x to make this > work...? Do you also hav a problem with 4.19.y? How about 5.2.y? If not, can you do 'git bisect' to find the patch that fixes the issue? thanks, greg k-h
Re: [PATCH 1/1] genirq: Properly pair kobject_del with kobject_add
On Thu, Aug 01, 2019 at 11:53:53PM +, Michael Kelley wrote:
> If alloc_descs fails before irq_sysfs_init has run, free_desc in the
> cleanup path will call kobject_del even though the kobject has not
> been added with kobject_add. Fix this by making the call to
> kobject_del conditional on whether irq_sysfs_init has run.
>
> This problem surfaced because commit aa30f47cf666
> ("kobject: Add support for default attribute groups to kobj_type")
> makes kobject_del stricter about pairing with kobject_add. If the
> pairing is incorrrect, a WARNING and backtrace occur in
> sysfs_remove_group because there is no parent.
>
> Fixes: ecb3f394c5db ("genirq: Expose interrupt information through sysfs")
> Signed-off-by: Michael Kelley
> ---
> kernel/irq/irqdesc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 9484e88..5447760 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -438,7 +438,8 @@ static void free_desc(unsigned int irq)
>* The sysfs entry must be serialized against a concurrent
>* irq_sysfs_init() as well.
>*/
> - kobject_del(>kobj);
> + if (irq_kobj_base)
> + kobject_del(>kobj);
But now you leak the memory of desc as there is no chance it could be
freed, because the kobject release function is never called :(
Relying on irq_kobj_bas to be present or not seems like an odd test
here.
thanks,
greg k-h
Re: [PATCH v10 0/6] Introduced new Cadence USBSS DRD Driver.
On Mon, Jul 22, 2019 at 01:56:45PM +0200, Pavel Machek wrote: > Hi! > > > > >> This patch introduce new Cadence USBSS DRD driver to linux kernel. > > > >> > > > >> The Cadence USBSS DRD Controller is a highly configurable IP Core which > > > >> can be instantiated as Dual-Role Device (DRD), Peripheral Only and > > > >> Host Only (XHCI)configurations. > > > > > > > >I see you are using debugfs to select between DRD, peripheral-onlyh and > > > >XHCI... > > > > > > > >Is that good idea? > > > > > > Yes driver allows selecting dr_mode by debugfs. Controller also support > > > such functionality > > > so I don't understand why would it not be a good idea. > > > > > > I personally use this for testing but it can be used to limit controller > > > functionality without > > > recompiling kernel. > > > > debugfs is ONLY for debugging, never rely on it being enabled, or > > mounted, on a system in order to have any normal operation happen. > > > > So for testing, yes, this is fine. If this is going to be the normal > > api/interface for how to control this driver, no, that is not acceptable > > at all. > > It makes a lot of sense for end-user to toggle this... for example > when he is lacking right cable for proper otg detection. As it is > third driver offering this functionality, I believe we should stop > treating it as debugging. Then it needs to get out of debugfs, as again, that can not be used for any normal end-user operation. thanks, greg k-h
Re: [PATCH nvmem 1/1] nvmem: imx: add i.MX8QM platform support
On Mon, Jul 15, 2019 at 05:34:47AM +, Andy Duan wrote: > Ping... It's the middle of the merge window, we can't do anything with any patches until after that. Please be patient. greg k-h
Re: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration
On Fri, Jul 12, 2019 at 09:11:57PM +0800, Xiaoming Ni wrote:
> On 2019/7/11 21:57, Vasily Averin wrote:
> > On 7/11/19 4:55 AM, Nixiaoming wrote:
> >> On Wed, July 10, 2019 1:49 PM Vasily Averin wrote:
> >>> On 7/10/19 6:09 AM, Xiaoming Ni wrote:
> Registering the same notifier to a hook repeatedly can cause the hook
> list to form a ring or lose other members of the list.
> >>>
> >>> I think is not enough to _prevent_ 2nd register attempt,
> >>> it's enough to detect just attempt and generate warning to mark host in
> >>> bad state.
> >>>
> >>
> >> Duplicate registration is prevented in my patch, not just "mark host in
> >> bad state"
> >>
> >> Duplicate registration is checked and exited in
> >> notifier_chain_cond_register()
> >>
> >> Duplicate registration was checked in notifier_chain_register() but only
> >> the alarm was triggered without exiting. added by commit 831246570d34692e
> >> ("kernel/notifier.c: double register detection")
> >>
> >> My patch is like a combination of 831246570d34692e and
> >> notifier_chain_cond_register(),
> >> which triggers an alarm and exits when a duplicate registration is
> >> detected.
> >>
> >>> Unexpected 2nd register of the same hook most likely will lead to 2nd
> >>> unregister,
> >>> and it can lead to host crash in any time:
> >>> you can unregister notifier on first attempt it can be too early, it can
> >>> be still in use.
> >>> on the other hand you can never call 2nd unregister at all.
> >>
> >> Since the member was not added to the linked list at the time of the
> >> second registration,
> >> no linked list ring was formed.
> >> The member is released on the first unregistration and -ENOENT on the
> >> second unregistration.
> >> After patching, the fault has been alleviated
> >
> > You are wrong here.
> > 2nd notifier's registration is a pure bug, this should never happen.
> > If you know the way to reproduce this situation -- you need to fix it.
> >
> > 2nd registration can happen in 2 cases:
> > 1) missed rollback, when someone forget to call unregister after
> > successfull registration,
> > and then tried to call register again. It can lead to crash for example
> > when according module will be unloaded.
> > 2) some subsystem is registered twice, for example from different
> > namespaces.
> > in this case unregister called during sybsystem cleanup in first namespace
> > will incorrectly remove notifier used
> > in second namespace, it also can lead to unexpacted behaviour.
> >
> So in these two cases, is it more reasonable to trigger BUG() directly when
> checking for duplicate registration ?
> But why does current notifier_chain_register() just trigger WARN() without
> exiting ?
> notifier_chain_cond_register() direct exit without triggering WARN() ?
It should recover from this, if it can be detected. The main point is
that not all apis have to be this "robust" when used within the kernel
as we do allow for the callers to know what they are doing :)
If this does not cause any additional problems or slow downs, it's
probably fine to add.
thanks,
greg k-h
Re: [PATCH] Adjust analogix chip driver location
On Wed, Jun 26, 2019 at 10:44:38AM +, Xin Ji wrote: > Move analogix chip ANX78XX bridge driver into "analogix" directory. > > Signed-off-by: Xin Ji > --- > drivers/gpu/drm/bridge/Kconfig | 10 - > drivers/gpu/drm/bridge/Makefile|3 +- > drivers/gpu/drm/bridge/analogix-anx78xx.c | 1485 > > drivers/gpu/drm/bridge/analogix-anx78xx.h | 710 -- > drivers/gpu/drm/bridge/analogix/Kconfig| 10 + > drivers/gpu/drm/bridge/analogix/Makefile |2 + > drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 1485 > > drivers/gpu/drm/bridge/analogix/analogix-anx78xx.h | 710 ++ > 8 files changed, 2208 insertions(+), 2207 deletions(-) > delete mode 100644 drivers/gpu/drm/bridge/analogix-anx78xx.c > delete mode 100644 drivers/gpu/drm/bridge/analogix-anx78xx.h > create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-anx78xx.h 'git format-patch -M' is usually a lot better to use when moving files around, as it shows you only any changes in the files, not a huge delete/add cycle. thanks, greg k-h
Re: [PATCH] drivers/usb/host/imx21-hcd.c: fix divide-by-zero in func nonisoc_etd_done
On Wed, Jun 05, 2019 at 02:02:40AM +, Duyanlin wrote: > > If the function usb_maxpacket(urb->dev, urb->pipe, usb_pipeout(urb->pipe)) > returns 0, that will cause a illegal divide-by-zero operation, unexpected > results may occur. > It is best to ensure that the denominator is non-zero before dividing by zero. Please wrap your changelog comments at 72 columns. > Signed-off-by: Yanlin Du This name HAS to match the From: line of your email. For that reason alone I can not take this patch. > --- > drivers/usb/host/imx21-hcd.c | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/host/imx21-hcd.c b/drivers/usb/host/imx21-hcd.c > index 6e3dad1..6a47f78 100644 > --- a/drivers/usb/host/imx21-hcd.c > +++ b/drivers/usb/host/imx21-hcd.c > @@ -1038,6 +1038,7 @@ static void nonisoc_etd_done(struct usb_hcd *hcd, int > etd_num) > int cc; > u32 bytes_xfrd; > int etd_done; > + unsigned int maxp; > > disactivate_etd(imx21, etd_num); > > @@ -1104,13 +1105,13 @@ static void nonisoc_etd_done(struct usb_hcd *hcd, int > etd_num) > break; > > case PIPE_BULK: > + maxp = usb_maxpacket(urb->dev, urb->pipe, > + usb_pipeout(urb->pipe)); How can this ever be 0? Don't we abort a lot earlier if a pipe length is 0? thanks, greg k-h
Re: [PATCH 4.19 144/187] selftests/bpf: skip verifier tests for unsupported program types
On Thu, May 23, 2019 at 03:46:26PM +0200, Daniel Borkmann wrote:
> On 05/23/2019 12:27 PM, Rantala, Tommi T. (Nokia - FI/Espoo) wrote:
> > On Thu, 2019-04-04 at 10:48 +0200, Greg Kroah-Hartman wrote:
> >> 4.19-stable review patch. If anyone has any objections, please let
> >> me know.
> >>
> >> --
> >>
> >> [ Upstream commit 8184d44c9a577a2f1842ed6cc844bfd4a9981d8e ]
> >>
> >> Use recently introduced bpf_probe_prog_type() to skip tests in the
> >> test_verifier() if bpf_verify_program() fails. The skipped test is
> >> indicated in the output.
> >
> > Hi, this patch added in 4.19.34 causes test_verifier build failure, as
> > bpf_probe_prog_type() is not available:
> >
> > gcc -Wall -O2 -I../../../include/uapi -I../../../lib -I../../../lib/bpf
> > -I../../../../include/generated -DHAVE_GENHDR
> > -I../../../includetest_verifier.c /root/linux-
> > 4.19.44/tools/testing/selftests/bpf/libbpf.a -lcap -lelf -lrt -lpthread
> > -o /root/linux-4.19.44/tools/testing/selftests/bpf/test_verifier
> > test_verifier.c: In function ‘do_test_single’:
> > test_verifier.c:12775:22: warning: implicit declaration of function
> > ‘bpf_probe_prog_type’; did you mean ‘bpf_program__set_type’? [-
> > Wimplicit-function-declaration]
> > if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
> > ^~~
> > bpf_program__set_type
> > /usr/bin/ld: /tmp/ccEtyLhk.o: in function `do_test_single':
> > test_verifier.c:(.text+0xa19): undefined reference to
> > `bpf_probe_prog_type'
> > collect2: error: ld returned 1 exit status
> > make[1]: *** [../lib.mk:152: /root/linux-
> > 4.19.44/tools/testing/selftests/bpf/test_verifier] Error 1
>
> Looks like this kselftest one got auto-selected for stable? It's not
> strictly needed, so totally fine to drop.
Ok, will go revert this, sorry for the breakage.
greg k-h
Re: [PATCH v1 1/3] x86/cpu: Create Zhaoxin processors architecture support file
On Thu, May 23, 2019 at 10:10:52AM +, TonyWWang-oc wrote:
> Add x86 architecture support for new Zhaoxin processors.
> Carve out initialization code needed by Zhaoxin processors into
> a separate compilation unit.
>
> To identify Zhaoxin CPU, add a new vendor type X86_VENDOR_ZHAOXIN
> for system recognition.
>
> Signed-off-by: TonyWWang
Some basic patch tips. Your From: line needs to match the signed-off-by
line, which is not true here. Also, please use your name with ' '
characters.
> +static void init_zhaoxin_cap(struct cpuinfo_x86 *c)
> +{
> + u32 lo, hi;
> +
> + /* Test for Extended Feature Flags presence */
> + if (cpuid_eax(0xC000) >= 0xC001) {
> + u32 tmp = cpuid_edx(0xC001);
This patch is totally corrupted, with leading spaces dropped and tabs
turned into spaces. Please read the email client documentation in the
kernel directory for how to fix your email client, or just use 'git
send-email' to send the patches out directly.
thanks,
greg k-h
Re: [PATCH v1 1/3] x86/cpu: Create Zhaoxin processors architecture support file
On Thu, May 23, 2019 at 10:10:52AM +, TonyWWang-oc wrote: > --- /dev/null > +++ b/arch/x86/kernel/cpu/zhaoxin.c > @@ -0,0 +1,178 @@ > +// SPDX-License-Identifier: GPL-2.0 Nice, but: > +/* > + * Zhaoxin Processor Support for Linux > + * > + * Copyright (c) 2019 Shanghai Zhaoxin Semiconductor Co., Ltd. > + * > + * Author: David Wang > + * > + * This file is licensed under the terms of the GNU General > + * License v2.0 or later. See file COPYING for details. That contridicts the first line of the file :( Please sort this out with your lawyers and resend it corrected. Also, gpl "boilerplate" text like this does not need to be in the file if you just include the spdx line. thanks, greg k-h
Re: [PATCH v1 1/3] x86/cpu: Create Zhaoxin processors architecture support file
On Thu, May 23, 2019 at 10:10:52AM +, TonyWWang-oc wrote: > Add x86 architecture support for new Zhaoxin processors. > Carve out initialization code needed by Zhaoxin processors into > a separate compilation unit. > > To identify Zhaoxin CPU, add a new vendor type X86_VENDOR_ZHAOXIN > for system recognition. > > Signed-off-by: TonyWWang > --- > MAINTAINERS | 6 ++ > arch/x86/Kconfig.cpu | 13 +++ > arch/x86/include/asm/processor.h | 3 +- > arch/x86/kernel/cpu/Makefile | 1 + > arch/x86/kernel/cpu/zhaoxin.c| 178 +++ > 5 files changed, 200 insertions(+), 1 deletion(-) > create mode 100644 arch/x86/kernel/cpu/zhaoxin.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 0c55b0f..cab21a4 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -17460,6 +17460,12 @@ Q: > https://patchwork.linuxtv.org/project/linux-media/list/ > S: Maintained > F: drivers/media/dvb-frontends/zd1301_demod* > +ZHAOXIN PROCESSOR SUPPORT > +M: TonyWWang > +L: linux-kernel@vger.kernel.org > +S: Maintained > +F: arch/x86/kernel/cpu/zhaoxin.c > + > ZPOOL COMPRESSED PAGE STORAGE API > M: Dan Streetman > L: linux...@kvack.org I think your email client ate the leading space here :( Anyway, you need a blank line before your entry. thanks, greg k-h
Re: [PATCH 4.14 09/69] x86: vdso: Use $LD instead of $CC to link
On Fri, Apr 26, 2019 at 06:34:22AM -0700, Nathan Chancellor wrote:
> On Fri, Apr 26, 2019 at 01:23:17PM +, Rantala, Tommi T. (Nokia -
> FI/Espoo) wrote:
> > On Fri, 2019-04-26 at 05:48 -0700, Nathan Chancellor wrote:
> > > On Fri, Apr 26, 2019 at 11:41:30AM +, Rantala, Tommi T. (Nokia -
> > > FI/Espoo) wrote:
> > > > On Mon, 2019-04-15 at 20:58 +0200, Greg Kroah-Hartman wrote:
> > > > > commit 379d98ddf41344273d9718556f761420f4dc80b3 upstream.
> > > > >
> > > >
> > > > Hi,
> > > >
> > > > With this patch in 4.14.112 build-id is now missing in vdso32.so:
> > > >
> > > > $ file arch/x86/entry/vdso/vdso*so*
> > > > arch/x86/entry/vdso/vdso32.so: ELF 32-bit LSB pie executable,
> > > > Intel
> > > > 80386, version 1 (SYSV), dynamically linked, stripped
> > > > arch/x86/entry/vdso/vdso32.so.dbg: ELF 32-bit LSB pie executable,
> > > > Intel
> > > > 80386, version 1 (SYSV), dynamically linked, with debug_info, not
> > > > stripped
> > > > arch/x86/entry/vdso/vdso64.so: ELF 64-bit LSB pie executable,
> > > > x86-
> > > > 64, version 1 (SYSV), dynamically linked,
> > > > BuildID[sha1]=d80730a5b561a3161e488a369d1c76c250b584b4, stripped
> > > > arch/x86/entry/vdso/vdso64.so.dbg: ELF 64-bit LSB pie executable,
> > > > x86-
> > > > 64, version 1 (SYSV), dynamically linked,
> > > > BuildID[sha1]=d80730a5b561a3161e488a369d1c76c250b584b4, with
> > > > debug_info, not stripped
> > > >
> > > >
> > > > Based on quick check, "$(call ld-option, --build-id)" fails due to
> > > > some
> > > > 32/64 bit mismatch, so the --build-id linker flag is not used when
> > > > linking vdso32.so
> > > >
> > > > Perhaps scripts/Kbuild.include is missing some change in 4.14.y to
> > > > make
> > > > this work properly.
> > > >
> > >
> > > Hi Tommi,
> > >
> > > This appears to be fixed by commit 0294e6f4a000 ("kbuild: simplify
> > > ld-option implementation") upstream. Could you test the attached
> > > backport and make sure everything works on your end? Assuming that it
> > > does, I will test the other stable releases and see if this is needed
> > > and send those backports along.
> >
> > Yes this patch fixes it. Many thanks!
>
> Thanks for verifying!
>
> Greg, attached are backports for that commit for 4.4, 4.9, and 4.14. It
> appeared in 4.16 so it is not needed with a newer version.
Now queued up, thanks!
greg k-h
Re: [PATCH 1/2] lib: add __sysfs_match_string_with_gaps() helper
On Wed, Apr 24, 2019 at 01:34:55PM +0100, Jonathan Cameron wrote:
> On Tue, 23 Apr 2019 06:38:44 +
> "Ardelean, Alexandru" wrote:
>
> > On Mon, 2019-04-22 at 23:06 +0200, Greg KH wrote:
> > >
> > >
> > > On Mon, Apr 22, 2019 at 11:32:56AM +0300, Alexandru Ardelean wrote:
> > > > This helper is similar to __sysfs_match_string() with the exception
> > > > that it
> > > > ignores NULL elements within the array.
> > >
> > > sysfs is "one value per file", why are you trying to write multiple
> > > things on a single line to a single sysfs file?
> > >
> > > Is IIO really that messy? :)
> > >
> >
> > Hmm, I don't think I understood the comment/question, or maybe I did not
> > formulate the comment properly.
> >
> > Maybe Jonathan can pitch-in here if I'm saying something wrong.
> >
> > So, in IIO there is `struct iio_enum` which is essentially a sysfs wrapper
> > for exposing an "enum" type to userspace via sysfs (which takes only one
> > value). This iio_enum type is basically a string-to-int mapping.
>
> >
> > Some example in C:
> >
> > enum {
> > ENUM0,
> > ENUM1,
> > ENUM5 = 5,
> > ENUM6,
> > ENUM7
> > };
> >
> >
> > /* Notice the gaps in the elements */
> > static const char * const item_strings[] = {
> > [ENUM0] = "mode0",
> > [ENUM1] = "mode1",
> > [ENUM5] = "mode5",
> > [ENUM6] = "mode6",
> > [ENUM7] = "mode7",
> > };
> >
> > static const struct iio_enum iio_enum1 = {
> > .items = item_strings,
> > .num_items = ARRAY_SIZE(item_strings),
> > .set = iio_enum1_set,
> > .get = iio_enum1_get,
> > };
> >
> >
> > The signature of the iio_enum1_set / iio_enum1_get is below:
> >
> > static int iio_enum1_set(struct iio_dev *indio_dev,
> > const struct iio_chan_spec *chan, unsigned int val);
> >
> > static int iio_enum1_get(struct iio_dev *indio_dev,
> > const struct iio_chan_spec *chan)
> >
> >
> > IIO core resolves the string-to-int mapping.
> > It uses __sysfs_match_string() to do that, but it requires that the list of
> > strings (and C enums) be contiguous.
> > This change [and V2 of this patch] introduces a
> > __sysfs_match_string_with_gaps() helper that ignores gaps (represented as
> > NULLs).
> >
> > For reference, __sysfs_match_string() returns -EINVAL on the first NULL in
> > the array of strings (regardless of the given array size).
> >
> > __sysfs_match_string_with_gaps() is typically helpful when C enums refer to
> > bitfields, or have some equivalence in HW.
> >
>
> You have described it well.
> Perhaps the issue is in the naming? Or more description is needed for the
> original
> patch.
>
> It's worth highlighting that the current help text for
> __sysfs_match_string has a description that says:
>
> /**
> * __sysfs_match_string - matches given string in an array
> * @array: array of strings
> * @n: number of strings in the array or -1 for NULL terminated arrays
> * @str: string to match with
> *
> * Returns index of @str in the @array or -EINVAL, just like match_string().
> * Uses sysfs_streq instead of strcmp for matching.
> */
>
> so one could argue that if you pass a value of n which is not -1 the function
> should not assume that any NULL terminates the array...
>
> So basically this new function is implementing what I would have assumed
> __sysfs_match_string would do, but doesn't.
Ok, yeah, I'm confused, I also thought this is what the original
function did.
Nevermind, no objection from me on this:
Acked-by: Greg Kroah-Hartman
