[PATCH] staging: erofs: fix potential double iput in erofs_read_super()
Some error cases like failing from d_make_root() will
cause double iput because d_make_root() also does iput
in its error path.
Signed-off-by: Chengguang Xu
---
Only compile tested.
drivers/staging/erofs/super.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 1c2eb69682ef..bd97679aacfc 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -420,13 +420,14 @@ static int erofs_read_super(struct super_block *sb,
errln("rootino(nid %llu) is not a directory(i_mode %o)",
ROOT_NID(sbi), inode->i_mode);
err = -EINVAL;
- goto err_isdir;
+ iput(inode);
+ goto err_iget;
}
sb->s_root = d_make_root(inode);
if (sb->s_root == NULL) {
err = -ENOMEM;
- goto err_makeroot;
+ goto err_iget;
}
/* save the device name to sbi */
@@ -452,10 +453,6 @@ static int erofs_read_super(struct super_block *sb,
*/
err_devname:
dput(sb->s_root);
-err_makeroot:
-err_isdir:
- if (sb->s_root == NULL)
- iput(inode);
err_iget:
#ifdef EROFS_FS_HAS_MANAGED_CACHE
iput(sbi->managed_cache);
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: ks7010: remove unnecessary parentheses
Remove unnecessary parentheses reported by checkpatch.
Signed-off-by: Matt McCoy
---
drivers/staging/ks7010/ks_hostif.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/ks7010/ks_hostif.c
b/drivers/staging/ks7010/ks_hostif.c
index 065bce1..d938b09 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -35,7 +35,7 @@ static inline u8 get_byte(struct ks_wlan_private *priv)
{
u8 data;
- data = *(priv->rxp)++;
+ data = *priv->rxp++;
/* length check in advance ! */
--(priv->rx_size);
return data;
@@ -171,7 +171,7 @@ int get_current_ap(struct ks_wlan_private *priv, struct
link_ap_info *ap_info)
"- rate_set_size=%d\n",
ap->bssid[0], ap->bssid[1], ap->bssid[2],
ap->bssid[3], ap->bssid[4], ap->bssid[5],
- &(ap->ssid.body[0]),
+ >ssid.body[0],
ap->rate_set.body[0], ap->rate_set.body[1],
ap->rate_set.body[2], ap->rate_set.body[3],
ap->rate_set.body[4], ap->rate_set.body[5],
@@ -732,7 +732,7 @@ void hostif_scan_indication(struct ks_wlan_private *priv)
netdev_dbg(priv->net_dev, " scan_ind_count=%d ::
aplist.size=%d\n",
priv->scan_ind_count, priv->aplist.size);
get_ap_information(priv, (struct ap_info *)(priv->rxp),
- &(priv->aplist.ap[priv->scan_ind_count -
1]));
+ >aplist.ap[priv->scan_ind_count - 1]);
priv->aplist.size = priv->scan_ind_count;
} else {
netdev_dbg(priv->net_dev, " count over :: scan_ind_count=%d\n",
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 13/14] staging: android: ion: Do not sync CPU cache on map/unmap
On Mon, 21 Jan 2019, Brian Starkey wrote:
> Hi,
>
> Sorry for being a bit sporadic on this. I was out travelling last week
> with little time for email.
>
> On Fri, Jan 18, 2019 at 11:16:31AM -0600, Andrew F. Davis wrote:
> > On 1/17/19 7:11 PM, Liam Mark wrote:
> > > On Thu, 17 Jan 2019, Andrew F. Davis wrote:
> > >
> > >> On 1/16/19 4:54 PM, Liam Mark wrote:
> > >>> On Wed, 16 Jan 2019, Andrew F. Davis wrote:
> > >>>
> > On 1/16/19 9:19 AM, Brian Starkey wrote:
> > > Hi :-)
> > >
> > > On Tue, Jan 15, 2019 at 12:40:16PM -0600, Andrew F. Davis wrote:
> > >> On 1/15/19 12:38 PM, Andrew F. Davis wrote:
> > >>> On 1/15/19 11:45 AM, Liam Mark wrote:
> > On Tue, 15 Jan 2019, Andrew F. Davis wrote:
> >
> > > On 1/14/19 11:13 AM, Liam Mark wrote:
> > >> On Fri, 11 Jan 2019, Andrew F. Davis wrote:
> > >>
> > >>> Buffers may not be mapped from the CPU so skip cache
> > >>> maintenance here.
> > >>> Accesses from the CPU to a cached heap should be bracketed with
> > >>> {begin,end}_cpu_access calls so maintenance should not be
> > >>> needed anyway.
> > >>>
> > >>> Signed-off-by: Andrew F. Davis
> > >>> ---
> > >>> drivers/staging/android/ion/ion.c | 7 ---
> > >>> 1 file changed, 4 insertions(+), 3 deletions(-)
> > >>>
> > >>> diff --git a/drivers/staging/android/ion/ion.c
> > >>> b/drivers/staging/android/ion/ion.c
> > >>> index 14e48f6eb734..09cb5a8e2b09 100644
> > >>> --- a/drivers/staging/android/ion/ion.c
> > >>> +++ b/drivers/staging/android/ion/ion.c
> > >>> @@ -261,8 +261,8 @@ static struct sg_table
> > >>> *ion_map_dma_buf(struct dma_buf_attachment *attachment,
> > >>>
> > >>> table = a->table;
> > >>>
> > >>> - if (!dma_map_sg(attachment->dev, table->sgl,
> > >>> table->nents,
> > >>> - direction))
> > >>> + if (!dma_map_sg_attrs(attachment->dev, table->sgl,
> > >>> table->nents,
> > >>> + direction,
> > >>> DMA_ATTR_SKIP_CPU_SYNC))
> > >>
> > >> Unfortunately I don't think you can do this for a couple reasons.
> > >> You can't rely on {begin,end}_cpu_access calls to do cache
> > >> maintenance.
> > >> If the calls to {begin,end}_cpu_access were made before the call
> > >> to
> > >> dma_buf_attach then there won't have been a device attached so
> > >> the calls
> > >> to {begin,end}_cpu_access won't have done any cache maintenance.
> > >>
> > >
> > > That should be okay though, if you have no attachments (or all
> > > attachments are IO-coherent) then there is no need for cache
> > > maintenance. Unless you mean a sequence where a non-io-coherent
> > > device
> > > is attached later after data has already been written. Does that
> > > sequence need supporting?
> >
> > Yes, but also I think there are cases where CPU access can happen
> > before
> > in Android, but I will focus on later for now.
> >
> > > DMA-BUF doesn't have to allocate the backing
> > > memory until map_dma_buf() time, and that should only happen
> > > after all
> > > the devices have attached so it can know where to put the buffer.
> > > So we
> > > shouldn't expect any CPU access to buffers before all the devices
> > > are
> > > attached and mapped, right?
> > >
> >
> > Here is an example where CPU access can happen later in Android.
> >
> > Camera device records video -> software post processing -> video
> > device
> > (who does compression of raw data) and writes to a file
> >
> > In this example assume the buffer is cached and the devices are
> > not
> > IO-coherent (quite common).
> >
> > >>>
> > >>> This is the start of the problem, having cached mappings of memory
> > >>> that
> > >>> is also being accessed non-coherently is going to cause issues one
> > >>> way
> > >>> or another. On top of the speculative cache fills that have to be
> > >>> constantly fought back against with CMOs like below; some coherent
> > >>> interconnects behave badly when you mix coherent and non-coherent
> > >>> access
> > >>> (snoop filters get messed up).
> > >>>
> > >>> The solution is to either always have the addresses marked
> > >>> non-coherent
> > >>> (like device memory, no-map carveouts), or if you really want to use
> > >>> regular system memory allocated at runtime, then all cached
> > >>> mappings of
> > >>> it need to be dropped,
Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes
On Tue, 22 Jan 2019, Andrew F. Davis wrote: > On 1/21/19 4:12 PM, Liam Mark wrote: > > On Mon, 21 Jan 2019, Christoph Hellwig wrote: > > > >> On Mon, Jan 21, 2019 at 11:44:10AM -0800, Liam Mark wrote: > >>> The main use case is for allowing clients to pass in > >>> DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance > >>> which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. In > >>> ION the buffers aren't usually accessed from the CPU so this allows > >>> clients to often avoid doing unnecessary cache maintenance. > >> > >> This can't work. The cpu can still easily speculate into this area. > > > > Can you provide more detail on your concern here. > > The use case I am thinking about here is a cached buffer which is accessed > > by a non IO-coherent device (quite a common use case for ION). > > > > Guessing on your concern: > > The speculative access can be an issue if you are going to access the > > buffer from the CPU after the device has written to it, however if you > > know you aren't going to do any CPU access before the buffer is again > > returned to the device then I don't think the speculative access is a > > concern. > > > >> Moreover in general these operations should be cheap if the addresses > >> aren't cached. > >> > > > > I am thinking of use cases with cached buffers here, so CMO isn't cheap. > > > > These buffers are cacheable, not cached, if you haven't written anything > the data wont actually be in cache. That's true > And in the case of speculative cache > filling the lines are marked clean. In either case the only cost is the > little 7 instruction loop calling the clean/invalidate instruction (dc > civac for ARMv8) for the cache-lines. Unless that is the cost you are > trying to avoid? > This is the cost I am trying to avoid and this comes back to our previous discussion. We have a coherent system cache so if you are doing this for every cache line on a large buffer it adds up with this work and the going to the bus. For example I believe 1080P buffers are 8MB, and 4K buffers are even larger. I also still think you would want to solve this properly such that invalidates aren't being done unnecessarily. > In that case if you are mapping and unmapping so much that the little > CMO here is hurting performance then I would argue your usage is broken > and needs to be re-worked a bit. > I am not sure I would say it is broken, the large buffers (example 1080P buffers) are mapped and unmapped on every frame. I don't think there is any clean way to avoid that in a pipelining framework, you could ask clients to keep the buffers dma mapped but there isn't necessarily a good time to tell them to unmap. It would be unfortunate to not consider this something legitimate for usespace to do in a pipelining use case. Requiring devices to stay attached doesn't seem very clean to me as there isn't necessarily a nice place to tell them when to detach. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes
On Tue, 22 Jan 2019, Andrew F. Davis wrote: > On 1/21/19 4:18 PM, Liam Mark wrote: > > On Mon, 21 Jan 2019, Andrew F. Davis wrote: > > > >> On 1/21/19 2:20 PM, Liam Mark wrote: > >>> On Mon, 21 Jan 2019, Andrew F. Davis wrote: > >>> > On 1/21/19 1:44 PM, Liam Mark wrote: > > On Mon, 21 Jan 2019, Christoph Hellwig wrote: > > > >> On Sat, Jan 19, 2019 at 08:50:41AM -0800, Laura Abbott wrote: > And who is going to decide which ones to pass? And who documents > which ones are safe? > > I'd much rather have explicit, well documented dma-buf flags that > might get translated to the DMA API flags, which are not error > checked, > not very well documented and way to easy to get wrong. > > >>> > >>> I'm not sure having flags in dma-buf really solves anything > >>> given drivers can use the attributes directly with dma_map > >>> anyway, which is what we're looking to do. The intention > >>> is for the driver creating the dma_buf attachment to have > >>> the knowledge of which flags to use. > >> > >> Well, there are very few flags that you can simply use for all calls of > >> dma_map*. And given how badly these flags are defined I just don't > >> want > >> people to add more places where they indirectly use these flags, as > >> it will be more than enough work to clean up the current mess. > >> > >> What flag(s) do you want to pass this way, btw? Maybe that is where > >> the problem is. > >> > > > > The main use case is for allowing clients to pass in > > DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance > > which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. > > In > > ION the buffers aren't usually accessed from the CPU so this allows > > clients to often avoid doing unnecessary cache maintenance. > > > > How can a client know that no CPU access has occurred that needs to be > flushed out? > > >>> > >>> I have left this to clients, but if they own the buffer they can have the > >>> knowledge as to whether CPU access is needed in that use case (example > >>> for > >>> post-processing). > >>> > >>> For example with the previous version of ION we left all decisions of > >>> whether cache maintenance was required up to the client, they would use > >>> the ION cache maintenance IOCTL to force cache maintenance only when it > >>> was required. > >>> In these cases almost all of the access was being done by the device and > >>> in the rare cases CPU access was required clients would initiate the > >>> required cache maintenance before and after the CPU access. > >>> > >> > >> I think we have different definitions of "client", I'm talking about the > >> DMA-BUF client (the importer), that is who can set this flag. It seems > >> you mean the userspace application, which has no control over this flag. > >> > > > > I am also talking about dma-buf clients, I am referring to both the > > userspace and kernel component of the client. For example our Camera ION > > client has both a usersapce and kernel component and they have ION > > buffers, which they control the access to, which may or may not be > > accessed by the CPU in certain uses cases. > > > > I know they often work together, but for this discussion it would be > good to keep kernel clients and usperspace clients separate. There are > three types of actors at play here, userspace clients, kernel clients, > and exporters. > > DMA-BUF only provides the basic sync primitive + mmap directly to > userspace, Well dma-buf does provide dma_buf_kmap/dma_buf_begin_cpu_access which allows the same fucntionality in the kernel, but I don't think that changes your argument. > both operations are fulfilled by the exporter. This patch is > about adding more control to the kernel side clients. The kernel side > clients cannot know what userspace or other kernel side clients have > done with the buffer, *only* the exporter has the whole picture. > > Therefor neither type of client should be deciding if the CPU needs > flushed or not, only the exporter, based on the type of buffer, the > current set attachments, and previous actions (is this first attachment, > CPU get access in-between, etc...) can make this decision. > > You goal seems to be to avoid unneeded CPU side CMOs when a device > detaches and another attaches with no CPU access in-between, right? > That's reasonable to me, but it must be the exporter who keeps track and > skips the CMO. This patch allows the client to tell the exporter the CMO > is not needed and that is not safe. > I agree it would be better have this logic in the exporter, but I just haven't heard an upstreamable way to make that work. But maybe to explore that a bit more. If we consider having CPU access with no devices attached a legitimate use case: The pipelining use case I
Re: [PATCH 4/4] staging: android: ion: Support for mapping with dma mapping attributes
On Mon, 21 Jan 2019, Brian Starkey wrote:
> Hi Liam,
>
> On Fri, Jan 18, 2019 at 10:37:47AM -0800, Liam Mark wrote:
> > Add support for configuring dma mapping attributes when mapping
> > and unmapping memory through dma_buf_map_attachment and
> > dma_buf_unmap_attachment.
> >
> > For example this will allow ION clients to skip cache maintenance, by
> > using DMA_ATTR_SKIP_CPU_SYNC, for buffers which are clean and haven't been
> > accessed by the CPU.
>
> How can a client know that the buffer won't be accessed by the CPU in
> the future though?
>
Yes, for use cases where you don't if it will be accessed in the future
then you would only use it to optimize the dma map path, but as I
mentioned in the other thread there are cases (such as in our Camera)
where we have complete ownership of buffers and do know if it will be
accessed in the future.
> I don't think we can push this decision to clients, because they are
> lacking information about what else is going on with the buffer. It
> needs to be done by the exporter, IMO.
>
I do agree it would be better to handle in the exporter, but in a
pipelining use case where there might not be any devices attached that
doesn't seem very doable.
> Thanks,
> -Brian
>
> >
> > Signed-off-by: Liam Mark
> > ---
> > drivers/staging/android/ion/ion.c | 7 ---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/android/ion/ion.c
> > b/drivers/staging/android/ion/ion.c
> > index 1fe633a7fdba..0aae845b20ba 100644
> > --- a/drivers/staging/android/ion/ion.c
> > +++ b/drivers/staging/android/ion/ion.c
> > @@ -268,8 +268,8 @@ static struct sg_table *ion_map_dma_buf(struct
> > dma_buf_attachment *attachment,
> > table = a->table;
> >
> > mutex_lock(>lock);
> > - if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
> > - direction)) {
> > + if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
> > + direction, attachment->dma_map_attrs)) {
> > mutex_unlock(>lock);
> > return ERR_PTR(-ENOMEM);
> > }
> > @@ -287,7 +287,8 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment
> > *attachment,
> > struct ion_buffer *buffer = attachment->dmabuf->priv;
> >
> > mutex_lock(>lock);
> > - dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
> > + dma_unmap_sg_attrs(attachment->dev, table->sgl, table->nents, direction,
> > + attachment->dma_map_attrs);
> > a->dma_mapped = false;
> > mutex_unlock(>lock);
> > }
> > --
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> >
> > ___
> > dri-devel mailing list
> > dri-de...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout
On Sat, Jan 19, 2019 at 11:29:12AM +0100, Hugo Lefeuvre wrote: > > > as far as I understand this code, freezable_schedule() avoids blocking the > > > freezer during the schedule() call, but in the end try_to_freeze() is > > > still > > > called so the result is the same, right? > > > I wonder why wait_event_freezable is not calling freezable_schedule(). > > > > It could be something subtle in my view. freezable_schedule() actually makes > > the freezer code not send a wake up to the sleeping task if a freeze > > happens, > > because the PF_FREEZER_SKIP flag is set, as you pointed. > > > > Whereas wait_event_freezable() which uses try_to_freeze() does not seem to > > have > > this behavior and the task will enter the freezer. So I'm a bit skeptical if > > your API will behave as expected (or at least consistently with other wait > > APIs). > > oh right, now it is clear to me: > > - schedule(); try_to_freeze() > > schedule() is called and the task enters sleep. Since PF_FREEZER_SKIP is > not set, the task wakes up as soon as try_to_freeze_tasks() is called. > Right after waking up the task calls try_to_freeze() which freezes it. > > - freezable_schedule() > > schedule() is called and the task enters sleep. Since PF_FREEZER_SKIP is > set, the task does not wake up when try_to_freeze_tasks() is called. This > is not a problem, the task can't "do anything which isn't allowed for a > frozen task" while sleeping[0]. > > When the task wakes up (timeout, or whatever other reason) it calls > try_to_freeze() which freezes it if the freeze is still underway. > > So if a freeze is triggered while the task is sleeping, a task executing > freezable_schedule() might or might not notice the freeze depending on how > long it sleeps. A task executing schedule(); try_to_freeze() will always > notice it. > > I might be wrong on that, but freezable_schedule() just seems like a > performance improvement to me. > > Now I fully agree with you that there should be a uniform definition of > "freezable" between wait_event_freezable and wait_event_freezable_hrtimeout. > This leaves me to the question: should I modify my definition of > wait_event_freezable_hrtimeout, or prepare a patch for wait_event_freezable ? > > If I am right with the performance thing, the latter might be worth > considering? > > Either way, this will be fixed in the v2. I agree, it is probably better to use freezable_schedule() for all freeze related wait APIs, and keep it consistent. Your analysis is convincing. Peter, what do you think? > > > That being said, I am not sure that the try_to_freeze() call does anything > > > in the vsoc case because there is no call to set_freezable() so the thread > > > still has PF_NOFREEZE... > > > > I traced this, and PF_NOFREEZE is not set by default for tasks. > > Well, I did not check this in practice and might be confused somewhere but > the documentation[1] says "kernel threads are not freezable by default. > However, a kernel thread may clear PF_NOFREEZE for itself by calling > set_freezable()". > > Looking at the kthreadd() definition it seems like new tasks have > PF_NOFREEZE set by default[2]. > > I'll take some time to check this in practice in the next days. > > Anyways, thanks for your time ! Yeah, no problem. thanks, - Joel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] Drivers: hv: vmbus: Add mutex lock to channel show functions
> From: Kimberly Brown
> Sent: Monday, January 21, 2019 10:43 PM
> > > @@ -1421,7 +1422,10 @@ static ssize_t vmbus_chan_attr_show(struct
> > > kobject *kobj,
> > > if (chan->state != CHANNEL_OPENED_STATE)
> > > return -EINVAL;
> > >
> > > - return attribute->show(chan, buf);
> > > + mutex_lock(_connection.channel_mutex);
> > > + ret = attribute->show(chan, buf);
> > > + mutex_unlock(_connection.channel_mutex);
> > > + return ret;
> > > }
> >
> > It looks this patch is already able to fix the original issue:
> > 6712cc9c2211 ("vmbus: don't return values for uninitalized channels"),
> > as chan->state can't be CHANNEL_OPENED_STATE when
> > channel->ringbuffer_page is not set up yet, or has been freed.
> > -- Dexuan
>
> I think that patch 6712cc9c2211 fixes the problem when the channel is
> not set up yet, but it doesn't fix the problem when the channel is being
> closed
Yeah, now I see your point.
> The channel could be freed after the check that "chan->state" is
> CHANNEL_OPENED_STATE, while the "attribute->show()" function is running.
IMO the channel struct itself can't be freed while the "attribute->show()"
function is running, because I suppose the sysfs interface should have an extra
reference to channel->kobj (see vmbus_add_channel_kobj()), so before the sysfs
files are removed, the channel struct itself can't disappear.
(I didn't check the related code very carefully, so I could be wrong. :-) )
But as you pointed, at least for sub-channels, channel->ringbuffer_page
can indeed disappear in vmbus_close() -> ... -> vmbus_free_ring(), and
the "attribute->show()" could crash when the race happens.
Adding channel_mutex here seems to be able to fix the race for
sub-channels, as the same channel_mutex is used in vmbus_disconnect_ring().
For a primary channel, vmbus_close() -> vmbus_free_ring() can still
free channel->ringbuffer_page without notifying the "attribute->show()".
We may also need to acquire/release the channel_mutex in vmbus_close()?
> Actually, there should be checks that "chan" is not null and that
> "chan->state" is CHANNEL_OPENED_STATE within the locked section. I'll
> need to fix that.
I suppose "chan" can not be NULL here (see the above).
Checking "chan->state" may not help to completely fix the race, because
there is no locking/synchronization code in
vmbus_close_internal() when we test and change "channel->state".
I guess we may need to check if channel->ringbuffer_page is NULL in
the "attribute->show()".
PS, to prove that a race condition does exist and can really cause a panic or
something, I usually add some msleep() delays in different paths so that I
can reproduce the crash every time I take a special action, e.g. when I read
the sysfs files of a NIC, I try to remove hv_netvsc driver. This way, I can
prove
a patch can indeed help, at least it can fix the crash which would happen
without the patch. :-)
-- Dexuan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 13/14] staging: android: ion: Do not sync CPU cache on map/unmap
Hello everyone,
Sincere apologies for chiming in a bit late here, but was off due to
some health issues.
Also, adding Daniel Vetter to the mix, since he has been one of the
core guys who shaped up dma-buf as it is today.
On Tue, 22 Jan 2019 at 02:51, Andrew F. Davis wrote:
On 1/21/19 5:22 AM, Brian Starkey wrote:
Hi,
Sorry for being a bit sporadic on this. I was out travelling last week
with little time for email.
On Fri, Jan 18, 2019 at 11:16:31AM -0600, Andrew F. Davis wrote:
On 1/17/19 7:11 PM, Liam Mark wrote:
On Thu, 17 Jan 2019, Andrew F. Davis wrote:
On 1/16/19 4:54 PM, Liam Mark wrote:
On Wed, 16 Jan 2019, Andrew F. Davis wrote:
On 1/16/19 9:19 AM, Brian Starkey wrote:
Hi :-)
On Tue, Jan 15, 2019 at 12:40:16PM
-0600, Andrew F. Davis wrote:
On 1/15/19 12:38 PM, Andrew F.
Davis wrote:
On 1/15/19 11:45 AM, Liam Mark wrote:
On Tue, 15 Jan 2019,
Andrew F. Davis wrote:
On 1/14/19 11:13 AM,
Liam Mark wrote:
On Fri, 11 Jan
2019, Andrew F. Davis wrote:
Buffers may
not be mapped from the CPU so skip cache maintenance here.
Accesses
from the CPU to a cached heap should be bracketed with
{begin,end}_cpu_access calls so maintenance should not be needed
anyway.
Signed-off-by: Andrew F. Davis
---
drivers/staging/android/ion/ion.c | 7 ---
1 file
changed, 4 insertions(+), 3 deletions(-)
diff --git
a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index
14e48f6eb734..09cb5a8e2b09 100644
---
a/drivers/staging/android/ion/ion.c
+++
b/drivers/staging/android/ion/ion.c
@@ -261,8
+261,8 @@ static struct sg_table *ion_map_dma_buf(struct
dma_buf_attachment *attachment,
table = a-table;
- if
(!dma_map_sg(attachment-dev, table-sgl, table-nents,
-
direction))
+ if
(!dma_map_sg_attrs(attachment-dev, table-sgl, table-nents,
+
direction, DMA_ATTR_SKIP_CPU_SYNC))
Unfortunately I
don't think you can do this for a couple reasons.
You can't rely
on {begin,end}_cpu_access calls to do cache maintenance.
If the calls to
{begin,end}_cpu_access were made before the call to
dma_buf_attach
then there won't have been a device attached so the calls
to
{begin,end}_cpu_access won't have done any cache maintenance.
That should be okay
though, if you have no attachments (or all
attachments are
IO-coherent) then there is no need for cache
maintenance. Unless
you mean a sequence where a non-io-coherent device
is attached later
after data has already been written. Does that
sequence need supporting?
Yes, but also I think
there are cases where CPU access can happen before
in Android, but I will
focus on later for now.
DMA-BUF doesn't have
to allocate the backing
memory until
map_dma_buf() time, and that should only happen after all
the devices have
attached so it can know where to put the buffer. So we
shouldn't expect any
CPU access to buffers before all the devices are
attached and mapped, right?
Here is an example where
CPU access can happen later in Android.
Camera device records
video - software post processing - video device
(who does compression of
raw data) and writes to a file
In this example assume
the buffer is cached and the devices are not
IO-coherent (quite common).
This is the start of the
problem, having cached mappings of memory that
is also being accessed
non-coherently is going to cause issues one way
or another. On top of the
speculative cache fills that have to be
constantly fought back
against with CMOs like below; some coherent
interconnects behave badly
when you mix coherent and non-coherent access
(snoop filters get messed up).
The solution is to either
always have the addresses marked non-coherent
(like device memory, no-map
carveouts), or if you really want to use
regular system memory
allocated at runtime, then all cached mappings of
it need to be dropped, even
the kernel logical address (area as painful
as that would be).
Ouch :-( I wasn't aware about these
potential interconnect issues. How
"real" is that? It seems that we
aren't really hitting that today on
real devices.
Sadly there is at least one real device
like this now (TI AM654). We
spent some time working with the ARM
interconnect spec designers to see
if this was allowed behavior, final
conclusion was mixing coherent and
non-coherent accesses is never a good
idea.. So we have been working to
try to minimize any cases of mixed
attributes [0], if a region is
coherent then everyone in the system
needs to treat it as such and
vice-versa, even clever CMO that work on
other systems wont save you
here. :(
[0]
https://github.com/ARM-software/arm-trusted-firmware/pull/1553
"Never a good idea" - but I think it should still be well defined by
the ARMv8 ARM (Section B2.8). Does this apply to your system?
"If the mismatched attributes for a memory location all assign the
same shareability attribute to
Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes
On 1/21/19 4:12 PM, Liam Mark wrote: > On Mon, 21 Jan 2019, Christoph Hellwig wrote: > >> On Mon, Jan 21, 2019 at 11:44:10AM -0800, Liam Mark wrote: >>> The main use case is for allowing clients to pass in >>> DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance >>> which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. In >>> ION the buffers aren't usually accessed from the CPU so this allows >>> clients to often avoid doing unnecessary cache maintenance. >> >> This can't work. The cpu can still easily speculate into this area. > > Can you provide more detail on your concern here. > The use case I am thinking about here is a cached buffer which is accessed > by a non IO-coherent device (quite a common use case for ION). > > Guessing on your concern: > The speculative access can be an issue if you are going to access the > buffer from the CPU after the device has written to it, however if you > know you aren't going to do any CPU access before the buffer is again > returned to the device then I don't think the speculative access is a > concern. > >> Moreover in general these operations should be cheap if the addresses >> aren't cached. >> > > I am thinking of use cases with cached buffers here, so CMO isn't cheap. > These buffers are cacheable, not cached, if you haven't written anything the data wont actually be in cache. And in the case of speculative cache filling the lines are marked clean. In either case the only cost is the little 7 instruction loop calling the clean/invalidate instruction (dc civac for ARMv8) for the cache-lines. Unless that is the cost you are trying to avoid? In that case if you are mapping and unmapping so much that the little CMO here is hurting performance then I would argue your usage is broken and needs to be re-worked a bit. Andrew > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] dma-buf: add support for mapping with dma mapping attributes
On 1/21/19 4:18 PM, Liam Mark wrote: > On Mon, 21 Jan 2019, Andrew F. Davis wrote: > >> On 1/21/19 2:20 PM, Liam Mark wrote: >>> On Mon, 21 Jan 2019, Andrew F. Davis wrote: >>> On 1/21/19 1:44 PM, Liam Mark wrote: > On Mon, 21 Jan 2019, Christoph Hellwig wrote: > >> On Sat, Jan 19, 2019 at 08:50:41AM -0800, Laura Abbott wrote: And who is going to decide which ones to pass? And who documents which ones are safe? I'd much rather have explicit, well documented dma-buf flags that might get translated to the DMA API flags, which are not error checked, not very well documented and way to easy to get wrong. >>> >>> I'm not sure having flags in dma-buf really solves anything >>> given drivers can use the attributes directly with dma_map >>> anyway, which is what we're looking to do. The intention >>> is for the driver creating the dma_buf attachment to have >>> the knowledge of which flags to use. >> >> Well, there are very few flags that you can simply use for all calls of >> dma_map*. And given how badly these flags are defined I just don't want >> people to add more places where they indirectly use these flags, as >> it will be more than enough work to clean up the current mess. >> >> What flag(s) do you want to pass this way, btw? Maybe that is where >> the problem is. >> > > The main use case is for allowing clients to pass in > DMA_ATTR_SKIP_CPU_SYNC in order to skip the default cache maintenance > which happens in dma_buf_map_attachment and dma_buf_unmap_attachment. In > ION the buffers aren't usually accessed from the CPU so this allows > clients to often avoid doing unnecessary cache maintenance. > How can a client know that no CPU access has occurred that needs to be flushed out? >>> >>> I have left this to clients, but if they own the buffer they can have the >>> knowledge as to whether CPU access is needed in that use case (example for >>> post-processing). >>> >>> For example with the previous version of ION we left all decisions of >>> whether cache maintenance was required up to the client, they would use >>> the ION cache maintenance IOCTL to force cache maintenance only when it >>> was required. >>> In these cases almost all of the access was being done by the device and >>> in the rare cases CPU access was required clients would initiate the >>> required cache maintenance before and after the CPU access. >>> >> >> I think we have different definitions of "client", I'm talking about the >> DMA-BUF client (the importer), that is who can set this flag. It seems >> you mean the userspace application, which has no control over this flag. >> > > I am also talking about dma-buf clients, I am referring to both the > userspace and kernel component of the client. For example our Camera ION > client has both a usersapce and kernel component and they have ION > buffers, which they control the access to, which may or may not be > accessed by the CPU in certain uses cases. > I know they often work together, but for this discussion it would be good to keep kernel clients and usperspace clients separate. There are three types of actors at play here, userspace clients, kernel clients, and exporters. DMA-BUF only provides the basic sync primitive + mmap directly to userspace, both operations are fulfilled by the exporter. This patch is about adding more control to the kernel side clients. The kernel side clients cannot know what userspace or other kernel side clients have done with the buffer, *only* the exporter has the whole picture. Therefor neither type of client should be deciding if the CPU needs flushed or not, only the exporter, based on the type of buffer, the current set attachments, and previous actions (is this first attachment, CPU get access in-between, etc...) can make this decision. You goal seems to be to avoid unneeded CPU side CMOs when a device detaches and another attaches with no CPU access in-between, right? That's reasonable to me, but it must be the exporter who keeps track and skips the CMO. This patch allows the client to tell the exporter the CMO is not needed and that is not safe. Andrew > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel
Philipp,
On Mon, Jan 21, 2019 at 12:49:10PM +0100, Philipp Zabel wrote:
> Hi,
>
> On Fri, 2019-01-18 at 17:04 -0800, Steve Longerbeam wrote:
> > Disable the SMFC before disabling the IDMA channel, instead of after,
> > in csi_idmac_unsetup().
> >
> > This fixes a complete system hard lockup on the SabreAuto when streaming
> > from the ADV7180, by repeatedly sending a stream off immediately followed
> > by stream on:
> >
> > while true; do v4l2-ctl -d4 --stream-mmap --stream-count=3; done
> >
> > Eventually this either causes the system lockup or EOF timeouts at all
> > subsequent stream on, until a system reset.
> >
> > The lockup occurs when disabling the IDMA channel at stream off. Stopping
> > the video data stream entering the IDMA channel before disabling the
> > channel itself appears to be a reliable fix for the hard lockup. That can
> > be done either by disabling the SMFC or the CSI before disabling the
> > channel. Disabling the SMFC before the channel is the easiest solution,
> > so do that.
> >
> > Fixes: 4a34ec8e470cb ("[media] media: imx: Add CSI subdev driver")
> >
> > Suggested-by: Peter Seiderer
> > Reported-by: Gaël PORTAY
> > Signed-off-by: Steve Longerbeam
>
> Gaël, could we get a Tested-by: for this as well?
>
I have tested the patchset v4 (not that one), and it has passed my test.
Regards,
Gael
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: possible deadlock in vfs_fallocate
Hello, syzbot has tested the proposed patch and the reproducer did not trigger crash: Reported-and-tested-by: syzbot+148c2885d71194f18...@syzkaller.appspotmail.com Tested on: commit: 48b161983ae5 Merge tag 'xarray-5.0-rc3' of git://git.infra.. git tree: upstream kernel config: https://syzkaller.appspot.com/x/.config?x=dd3a020d055fa2e8 compiler: gcc (GCC) 9.0.0 20181231 (experimental) patch: https://syzkaller.appspot.com/x/patch.diff?x=16e6d908c0 Note: testing is done by a robot and is best-effort only. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8723bs: fix indentation issue with return statement
From: Colin Ian King
A return statement is indented incorrectly, fix this.
Signed-off-by: Colin Ian King
---
drivers/staging/rtl8723bs/core/rtw_xmit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c
b/drivers/staging/rtl8723bs/core/rtw_xmit.c
index 625e67f39889..094d61bcb469 100644
--- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
+++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
@@ -2389,7 +2389,7 @@ sint xmitframe_enqueue_for_sleeping_sta(struct adapter
*padapter, struct xmit_fr
if (check_fwstate(pmlmepriv, WIFI_AP_STATE) == false) {
DBG_COUNTER(padapter->tx_logs.core_tx_ap_enqueue_warn_fwstate);
- return ret;
+ return ret;
}
/*
if (pattrib->psta)
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: possible deadlock in vfs_fallocate
On Wed, May 9, 2018 at 6:57 PM 'Todd Kjos' via syzkaller-bugs
wrote:
>
> +Joel Fernandes
>
> On Wed, May 9, 2018 at 12:55 AM Eric Biggers wrote:
>>
>> [+ashmem maintainers]
>>
>> On Sun, Apr 29, 2018 at 10:00:03AM -0700, syzbot wrote:
>> > Hello,
>> >
>> > syzbot hit the following crash on upstream commit
>> > cdface5209349930ae1b51338763c8e029971b97 (Sun Apr 29 03:07:21 2018 +)
>> > Merge tag 'for_linus_stable' of
>> > git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4
>> > syzbot dashboard link:
>> > https://syzkaller.appspot.com/bug?extid=148c2885d71194f18d28
>> >
>> > C reproducer: https://syzkaller.appspot.com/x/repro.c?id=5054004375584768
>> > syzkaller reproducer:
>> > https://syzkaller.appspot.com/x/repro.syz?id=6438048191479808
>> > Raw console output:
>> > https://syzkaller.appspot.com/x/log.txt?id=5404215203594240
>> > Kernel config:
>> > https://syzkaller.appspot.com/x/.config?id=7043958930931867332
>> > compiler: gcc (GCC) 8.0.1 20180413 (experimental)
Let's test Tetsuo's patch
#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
master
>> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
>> > Reported-by: syzbot+148c2885d71194f18...@syzkaller.appspotmail.com
>> > It will help syzbot understand when the bug is fixed. See footer for
>> > details.
>> > If you forward the report, please keep this part and the footer.
>> >
>> > random: sshd: uninitialized urandom read (32 bytes read)
>> > random: sshd: uninitialized urandom read (32 bytes read)
>> > random: sshd: uninitialized urandom read (32 bytes read)
>> >
>> > ==
>> > WARNING: possible circular locking dependency detected
>> > 4.17.0-rc2+ #23 Not tainted
>> > --
>> > syz-executor715/4492 is trying to acquire lock:
>> > (ptrval) (sb_writers#6){.+.+}, at: file_start_write
>> > include/linux/fs.h:2718 [inline]
>> > (ptrval) (sb_writers#6){.+.+}, at: vfs_fallocate+0x5be/0x8d0
>> > fs/open.c:318
>> >
>> > but task is already holding lock:
>> > (ptrval) (ashmem_mutex){+.+.}, at: ashmem_shrink_scan+0xac/0x560
>> > drivers/staging/android/ashmem.c:440
>> >
>> > which lock already depends on the new lock.
>> >
>> >
>> > the existing dependency chain (in reverse order) is:
>> >
>> > -> #3 (ashmem_mutex){+.+.}:
>> >__mutex_lock_common kernel/locking/mutex.c:756 [inline]
>> >__mutex_lock+0x16d/0x17f0 kernel/locking/mutex.c:893
>> >mutex_lock_nested+0x16/0x20 kernel/locking/mutex.c:908
>> >ashmem_mmap+0x53/0x460 drivers/staging/android/ashmem.c:361
>> >call_mmap include/linux/fs.h:1789 [inline]
>> >mmap_region+0xd13/0x1820 mm/mmap.c:1723
>> >do_mmap+0xc79/0x11d0 mm/mmap.c:1494
>> >do_mmap_pgoff include/linux/mm.h:2237 [inline]
>> >vm_mmap_pgoff+0x1fb/0x2a0 mm/util.c:357
>> >ksys_mmap_pgoff+0x4c9/0x640 mm/mmap.c:1544
>> >__do_sys_mmap arch/x86/kernel/sys_x86_64.c:100 [inline]
>> >__se_sys_mmap arch/x86/kernel/sys_x86_64.c:91 [inline]
>> >__x64_sys_mmap+0xe9/0x1b0 arch/x86/kernel/sys_x86_64.c:91
>> >do_syscall_64+0x1b1/0x800 arch/x86/entry/common.c:287
>> >entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> >
>> > -> #2 (>mmap_sem){}:
>> >__might_fault+0x155/0x1e0 mm/memory.c:4555
>> >_copy_to_user+0x30/0x110 lib/usercopy.c:25
>> >copy_to_user include/linux/uaccess.h:155 [inline]
>> >filldir+0x1ea/0x3a0 fs/readdir.c:196
>> >dir_emit_dot include/linux/fs.h:3378 [inline]
>> >dir_emit_dots include/linux/fs.h:3389 [inline]
>> >dcache_readdir+0x13a/0x620 fs/libfs.c:192
>> >iterate_dir+0x4b0/0x5d0 fs/readdir.c:51
>> >__do_sys_getdents fs/readdir.c:231 [inline]
>> >__se_sys_getdents fs/readdir.c:212 [inline]
>> >__x64_sys_getdents+0x293/0x4e0 fs/readdir.c:212
>> >do_syscall_64+0x1b1/0x800 arch/x86/entry/common.c:287
>> >entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> >
>> > -> #1 (>s_type->i_mutex_key#11){}:
>> >down_write+0x87/0x120 kernel/locking/rwsem.c:70
>> >inode_lock include/linux/fs.h:713 [inline]
>> >do_last fs/namei.c:3274 [inline]
>> >path_openat+0x123b/0x4e20 fs/namei.c:3501
>> >do_filp_open+0x249/0x350 fs/namei.c:3535
>> >do_sys_open+0x56f/0x740 fs/open.c:1093
>> >__do_sys_open fs/open.c: [inline]
>> >__se_sys_open fs/open.c:1106 [inline]
>> >__x64_sys_open+0x7e/0xc0 fs/open.c:1106
>> >do_syscall_64+0x1b1/0x800 arch/x86/entry/common.c:287
>> >entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> >
>> > -> #0 (sb_writers#6){.+.+}:
>> >lock_acquire+0x1dc/0x520 kernel/locking/lockdep.c:3920
>> >percpu_down_read_preempt_disable include/linux/percpu-rwsem.h:36
>> > [inline]
>> >percpu_down_read include/linux/percpu-rwsem.h:59 [inline]
>> >
Re: [PATCH] staging: netlogic: replace ---help--- with help in Kconfig
On Sun, Jan 20, 2019 at 09:40:39PM +0530, Bharath Vedartham wrote: > This patch fixes the checkpatch.pl warning: > > WARNING: prefer 'help' over '---help---' for new help texts > > Signed-off-by: Bharath Vedartham > --- > drivers/staging/netlogic/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Any reason you sent this twice? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel
On Mon, 2019-01-21 at 10:46 -0800, Steve Longerbeam wrote: > > On 1/21/19 10:43 AM, Steve Longerbeam wrote: > > > > > > On 1/21/19 3:49 AM, Philipp Zabel wrote: > > > Also ipu_smfc_disable is refcounted, so if the other CSI is capturing > > > simultaneously, this change has no effect. > > > > Sigh, you're right. Let me go back to disabling the CSI before the > > channel, the CSI enable/disable is not refcounted (it doesn't need to > > be since it is single use) so it doesn't have this problem. > > > > Should we drop this patch or keep it (with a big comment)? By only > > changing the disable order to "CSI then channel", the hang is reliably > > fixed from my and Gael's testing, but my concern is that by not > > disabling the SMFC before the channel, the SMFC could still empty its > > FIFO to the channel's internal FIFO and still create a hang. > > Well, as you said it will have no effect if both CSI's are streaming > with the SMFC, in which case the danger would still exist. Perhaps it > would be best to just drop this patch. Hm, if we can't guarantee the intended effect with this patch, and stopping the CSI first helps reliably, it's indeed better to just do that instead. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
