[PATCH] staging: rts5208: Fixed checkpath warning.
This patch solves the following checkpatch.pl's message in drivers/staging/rts5208/rtsx_transport.c:397. WARNING: line over 80 characters + option = RTSX_SG_VALID | RTSX_SG_END | RTSX_SG_TRANS_DATA; Signed-off-by: Prakhar Sinha --- drivers/staging/rts5208/rtsx_transport.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/staging/rts5208/rtsx_transport.c b/drivers/staging/rts5208/rtsx_transport.c index 8277d7895608..3fc83875fe7c 100644 --- a/drivers/staging/rts5208/rtsx_transport.c +++ b/drivers/staging/rts5208/rtsx_transport.c @@ -394,7 +394,8 @@ static int rtsx_transfer_sglist_adma_partial(struct rtsx_chip *chip, u8 card, *index = *index + 1; } if ((i == (sg_cnt - 1)) || !resid) - option = RTSX_SG_VALID | RTSX_SG_END | RTSX_SG_TRANS_DATA; + option = RTSX_SG_VALID | RTSX_SG_END | +RTSX_SG_TRANS_DATA; else option = RTSX_SG_VALID | RTSX_SG_TRANS_DATA; -- 2.20.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rts5208: Fix checkpath warning
This patch fixes the following checkpath warning in the file drivers/staging/rts5208/rtsx_transport.c:546 WARNING: line over 80 characters + option = RTSX_SG_VALID | RTSX_SG_END | RTSX_SG_TRANS_DATA; Signed-off-by: P SAI PRASANTH --- drivers/staging/rts5208/rtsx_transport.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/staging/rts5208/rtsx_transport.c b/drivers/staging/rts5208/rtsx_transport.c index 8277d78..495fa60 100644 --- a/drivers/staging/rts5208/rtsx_transport.c +++ b/drivers/staging/rts5208/rtsx_transport.c @@ -542,7 +542,8 @@ static int rtsx_transfer_sglist_adma(struct rtsx_chip *chip, u8 card, (unsigned int)addr, len); if (j == (sg_cnt - 1)) - option = RTSX_SG_VALID | RTSX_SG_END | RTSX_SG_TRANS_DATA; + option = RTSX_SG_VALID | RTSX_SG_END | +RTSX_SG_TRANS_DATA; else option = RTSX_SG_VALID | RTSX_SG_TRANS_DATA; -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/8] media: cedrus: Detect first slice of a frame
On 8/22/19 9:44 PM, Jernej Skrabec wrote:
> When codec supports multiple slices in one frame, VPU has to know when
> first slice of each frame is being processed, presumably to correctly
> clear/set data in auxiliary buffers.
>
> Add first_slice field to cedrus_run structure and set it according to
> timestamps of capture and output buffers. If timestamps are different,
> it's first slice and viceversa.
>
> Signed-off-by: Jernej Skrabec
> ---
> drivers/staging/media/sunxi/cedrus/cedrus.h | 1 +
> drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++
> 2 files changed, 3 insertions(+)
>
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
> b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index 2f017a651848..32cb38e541c6 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -70,6 +70,7 @@ struct cedrus_mpeg2_run {
> struct cedrus_run {
> struct vb2_v4l2_buffer *src;
> struct vb2_v4l2_buffer *dst;
> + bool first_slice;
>
> union {
> struct cedrus_h264_run h264;
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> index 56ca4c9ad01c..d7b54accfe83 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> @@ -31,6 +31,8 @@ void cedrus_device_run(void *priv)
>
> run.src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> run.dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> + run.first_slice =
> + run.src->vb2_buf.timestamp != run.dst->vb2_buf.timestamp;
This is almost correct. To handle the corner case where no timestamp
was ever copied to run.dst->vb2_buf you need this check:
run.first_slice = !run.dst->vb2_buf.copied_timestamp ||
run.src->vb2_buf.timestamp != run.dst->vb2_buf.timestamp;
Regards,
Hans
>
> /* Apply request(s) controls if needed. */
> src_req = run.src->vb2_buf.req_obj.req;
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH AUTOSEL 4.19 11/29] binder: take read mode of mmap_sem in binder_alloc_free_page()
On 2019-08-30 08:29:10, Greg Kroah-Hartman wrote:
> On Thu, Aug 29, 2019 at 10:13:20AM -0500, Tyler Hicks wrote:
> > Hello, Sasha!
> >
> > On 2019-08-29 06:49:51, Sasha Levin wrote:
> > > From: Tyler Hicks
> > >
> > > [ Upstream commit 60d4885710836595192c42d3e04b27551d30ec91 ]
> > >
> > > Restore the behavior of locking mmap_sem for reading in
> > > binder_alloc_free_page(), as was first done in commit 3013bf62b67a
> > > ("binder: reduce mmap_sem write-side lock"). That change was
> > > inadvertently reverted by commit 5cec2d2e5839 ("binder: fix race between
> > > munmap() and direct reclaim").
> > >
> > > In addition, change the name of the label for the error path to
> > > accurately reflect that we're taking the lock for reading.
> > >
> > > Backporting note: This fix is only needed when *both* of the commits
> > > mentioned above are applied. That's an unlikely situation since they
> > > both landed during the development of v5.1 but only one of them is
> > > targeted for stable.
> >
> > This patch isn't meant to be applied to 4.19 since commit 3013bf62b67a
> > ("binder: reduce mmap_sem write-side lock") was never brought back to
> > 4.19.
> >
> > Tyler
> >
> > >
> > > Fixes: 5cec2d2e5839 ("binder: fix race between munmap() and direct
> > > reclaim")
>
> But this commit is in 4.19.49
That's correct but commit 3013bf62b67a isn't present in 4.19.y. See the
"Backporting note" above.
Tyler
>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH AUTOSEL 4.14 05/14] binder: take read mode of mmap_sem in binder_alloc_free_page()
On 2019-08-30 08:23:49, Greg Kroah-Hartman wrote:
> On Thu, Aug 29, 2019 at 10:10:52AM -0500, Tyler Hicks wrote:
> > Hello, Sasha!
> >
> > On 2019-08-29 06:50:34, Sasha Levin wrote:
> > > From: Tyler Hicks
> > >
> > > [ Upstream commit 60d4885710836595192c42d3e04b27551d30ec91 ]
> > >
> > > Restore the behavior of locking mmap_sem for reading in
> > > binder_alloc_free_page(), as was first done in commit 3013bf62b67a
> > > ("binder: reduce mmap_sem write-side lock"). That change was
> > > inadvertently reverted by commit 5cec2d2e5839 ("binder: fix race between
> > > munmap() and direct reclaim").
> > >
> > > In addition, change the name of the label for the error path to
> > > accurately reflect that we're taking the lock for reading.
> > >
> > > Backporting note: This fix is only needed when *both* of the commits
> > > mentioned above are applied. That's an unlikely situation since they
> > > both landed during the development of v5.1 but only one of them is
> > > targeted for stable.
> >
> > This patch isn't meant to be applied to 4.14 since commit 3013bf62b67a
> > ("binder: reduce mmap_sem write-side lock") was never brought back to
> > 4.14.
>
> But the patch says:
> Fixes: 5cec2d2e5839 ("binder: fix race between munmap() and direct
> reclaim")
> and that commit is in 4.14.124.
This patch fixes 5cec2d2e5839 but only when 3013bf62b67a is also
applied. If 3013bf62b67a isn't present, this patch shouldn't be
backported. 3013bf62b67a isn't in 4.14.y.
Tyler
>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 00/24] erofs: promote erofs from staging v8
On 2019/8/15 12:41, Gao Xiang wrote: > [I strip the previous cover letter, the old one can be found in v6: > https://lore.kernel.org/r/20190802125347.166018-1-gaoxian...@huawei.com/] > > We'd like to submit a formal moving patch applied to staging tree > for 5.4, before that we'd like to hear if there are some ACKs, > suggestions or NAKs, objections of EROFS. Therefore, we can improve > it in this round or rethink about the whole thing. > > As related materials mentioned [1] [2], the goal of EROFS is to > save extra storage space with guaranteed end-to-end performance > for read-only files, which has better performance over exist Linux > compression filesystems based on fixed-sized output compression > and inplace decompression. It even has better performance in > a large compression ratio range compared with generic uncompressed > filesystems with proper CPU-storage combinations. And we think this > direction is correct and a dedicated kernel team is continuously / > actively working on improving it, enough testers and beta / end > users using it. > > EROFS has been applied to almost all in-service HUAWEI smartphones > (Yes, the number is still increasing by time) and it seems like > a success. It can be used in more wider scenarios. We think it's > useful for Linux / Android OS community and it's the time moving > out of staging. > > In order to get started, latest stable mkfs.erofs is available at > > git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git -b dev > > with README in the repository. > > We are still tuning sequential read performance for ultra-fast > speed NVME SSDs like Samsung 970PRO, but at least now you can > try on your PC with some data with proper compression ratio, > the latest Linux kernel, USB stick for convenience sake and > a not very old-fashioned CPU. There are also benchmarks available > in the above materials mentioned. > > EROFS is a self-contained filesystem driver. Although there are > still some TODOs to be more generic, we will actively keep on > developping / tuning EROFS with the evolution of Linux kernel > as the other in-kernel filesystems. > > As I mentioned before in LSF/MM 2019, in the future, we'd like > to generalize the decompression engine into a library for other > fses to use after the whole system is mature like fscrypt. > However, such metadata should be designed respectively for > each fs, and synchronous metadata read cost will be larger > than EROFS because of those ondisk limitation. Therefore EROFS > is still a better choice for read-only scenarios. > > EROFS is now ready for reviewing and moving, and the code is > already cleaned up as shiny floors... Please kindly take some > precious time, share your comments about EROFS and let us know > your opinion about this. It's really important for us since > generally speaking, we like to use Linux _in-tree_ stuffs rather > than lack of supported out-of-tree / orphan stuffs as well. EROFS proposes its very unique fixed-sized output compression and inplace decompression framework joining into the ecosystem of compression filesystem, I think it will enrich diversity of compression filesystem, and bring healthy competition there. I do believe this is the right time to promote erofs to fs/ directory, let it be the formal member of filesystem clubhouse. Acked-by: Chao Yu Thanks > > Thank you in advance, > Gao Xiang > > [1] > https://kccncosschn19eng.sched.com/event/Nru2/erofs-an-introduction-and-our-smartphone-practice-xiang-gao-huawei > [2] https://www.usenix.org/conference/atc19/presentation/gao > > Changelog from v7: > o keep up with the latest staging tree in addition to >the latest staging patch: >https://lore.kernel.org/r/20190814103705.60698-1-gaoxian...@huawei.com/ >- use EUCLEAN for fs corruption cases suggested by Pavel; >- turn EIO into EOPNOTSUPP for unsupported on-disk format; >- fix all misused ENOTSUPP into EOPNOTSUPP pointed out by Chao; > o update cover letter > > It can also be found in git at tag "erofs_2019-08-15" (will be shown later) > at: > https://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git/ > > and the latest fs code is available at: > > https://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git/tree/fs/erofs?h=erofs-outofstaging > > Changelog from v6: > o keep up with the latest staging patchset > > https://lore.kernel.org/linux-fsdevel/20190813023054.73126-1-gaoxian...@huawei.com/ >in order to fix the following cases: >- inline erofs_inode_is_data_compressed() in erofs_fs.h; >- remove incomplete cleancache; >- remove all BUG_ON in EROFS. > o Removing the file names from the comments at the top of the files >suggested by Stephen will be applied to the real moving patch later. > > Changelog from v5: > o keep up with "[PATCH v2] staging: erofs: updates according to > erofs-outofstaging v4" > > https://lore.kernel.org/lkml/20190731155752.210602-1-gaoxian...@huawei.com/ >w
Re: [PATCH] staging: exfat: add exfat filesystem code to staging
Hello, thank you for input! On Thursday 29 August 2019 19:35:06 Sasha Levin wrote: > On Thu, Aug 29, 2019 at 07:18:16PM -0400, Valdis Klētnieks wrote: > > On Thu, 29 Aug 2019 22:56:31 +0200, Pali Roh?r said: > > > > > I'm not really sure if this exfat implementation is fully suitable for > > > mainline linux kernel. > > > > > > In my opinion, proper way should be to implement exFAT support into > > > existing fs/fat/ code instead of replacing whole vfat/msdosfs by this > > > new (now staging) fat implementation. > > > > > In linux kernel we really do not need two different implementation of > > > VFAT32. > > > > This patch however does have one major advantage over "patch vfat to > > support exfat" - which is that the patch exists. > > > > If somebody comes forward with an actual "extend vfat to do exfat" patch, > > we should at that point have a discussion about relative merits > > This patch going into staging doesn't necessarily mean that one day > it'll get moved to fs/exfat/. It's very possible that the approach would > instead be to use the staging code for reference, build this > functionality in fs/fat/, and kill off the staging code when it's not > needed anymore. So, if current exfat code is just going to be "reference" how to implement exfat properly in fs/fat, is this correct usage of "staging"? I was in impression that staging is there for drivers which needs cleanup as they are not ready to be part of mainline. But not that it is for "random" implementation of something which is going to be just a "code example" or "reference" for final implementation which would be different. Maybe other people could clarify state of staging, if this is how staging should be used or not. > With regards to missing specs/docs/whatever - our main concern with this > release was that we want full interoperability, which is why the spec > was made public as-is without modifications from what was used > internally. There's no "secret sauce" that Microsoft is hiding here. Ok, if it was just drop of "current version" of documentation then it makes sense. > How about we give this spec/code time to get soaked and reviewed for a > bit, and if folks still feel (in a month or so?) that there are missing > bits of information related to exfat, I'll be happy to go back and try > to get them out as well. Basically external references in that released exFAT specification are unknown / not released yet. Like TexFAT. So if you have an input in MS could you forward request about these missing bits? Also, MS already released source code of FAT32 kernel driver (fastfat.sys) and from code can be seen that it does not match MS FAT specification, and include some "secret sauce" bits. Source code is on: https://github.com/microsoft/Windows-driver-samples/tree/master/filesys/fastfat In fastfat.sys there is e.g. usage of undocumented bits in directory entry or usage of undocumented bits for marking filesystem as "dirty" -- incorrectly unmounted. Would it be possible for MS to release in same way code of exFAT NT driver? I'm really a bit sceptical that exFAT MS implementation is really according to standard as I already known that FAT32 is not. Just to note that I did some investigation in FAT32 level how are handled volume labels and I found more bugs in current implementations (mtools, dosfstools, util-linux and even in fastfat.sys). Some references are on: https://www.spinics.net/lists/kernel/msg2640891.html Fixes for mtools, dosfstools and util-linux were already applied and fix for MS's fastfat.sys is pending in opened pull request: https://github.com/microsoft/Windows-driver-samples/pull/303 Could you please forward my pull request for fastfat.sys fixes to appropriate people/team in MS? -- Pali Rohár pali.ro...@gmail.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: add exfat filesystem code to staging
On Thursday 29 August 2019 19:18:16 Valdis Klētnieks wrote: > On Thu, 29 Aug 2019 22:56:31 +0200, Pali Roh?r said: > > > I'm not really sure if this exfat implementation is fully suitable for > > mainline linux kernel. > > > > In my opinion, proper way should be to implement exFAT support into > > existing fs/fat/ code instead of replacing whole vfat/msdosfs by this > > new (now staging) fat implementation. > > > In linux kernel we really do not need two different implementation of > > VFAT32. > > This patch however does have one major advantage over "patch vfat to > support exfat" - which is that the patch exists. I understand that this is advantage... > If somebody comes forward with an actual "extend vfat to do exfat" patch, > we should at that point have a discussion about relative merits ... but is this advantage such big that it should be merged even due to "horrible" code quality and lot of code/functionality duplication? In similar way there should be discussion about these pros and cons. -- Pali Rohár pali.ro...@gmail.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rts5208: remove redundant sd30_mode checks
From: Colin Ian King
There are two hunks of code that check if sd30_mode is true however
an earlier check in an outer code block on sd30_mode being false means
that sd30_mode can never be true at these points so these checks are
redundant. Remove the dead code.
Addresses-Coverity: ("Logically dead code")
Signed-off-by: Colin Ian King
---
drivers/staging/rts5208/sd.c | 28 ++--
1 file changed, 10 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
index a06045344301..25c31496757e 100644
--- a/drivers/staging/rts5208/sd.c
+++ b/drivers/staging/rts5208/sd.c
@@ -2573,17 +2573,13 @@ static int reset_sd(struct rtsx_chip *chip)
retval = sd_sdr_tuning(chip);
if (retval != STATUS_SUCCESS) {
- if (sd20_mode) {
+ retval = sd_init_power(chip);
+ if (retval != STATUS_SUCCESS)
goto status_fail;
- } else {
- retval = sd_init_power(chip);
- if (retval != STATUS_SUCCESS)
- goto status_fail;
- try_sdio = false;
- sd20_mode = true;
- goto switch_fail;
- }
+ try_sdio = false;
+ sd20_mode = true;
+ goto switch_fail;
}
sd_send_cmd_get_rsp(chip, SEND_STATUS, sd_card->sd_addr,
@@ -2598,17 +2594,13 @@ static int reset_sd(struct rtsx_chip *chip)
if (read_lba0) {
retval = sd_read_lba0(chip);
if (retval != STATUS_SUCCESS) {
- if (sd20_mode) {
+ retval = sd_init_power(chip);
+ if (retval != STATUS_SUCCESS)
goto status_fail;
- } else {
- retval = sd_init_power(chip);
- if (retval != STATUS_SUCCESS)
- goto status_fail;
- try_sdio = false;
- sd20_mode = true;
- goto switch_fail;
- }
+ try_sdio = false;
+ sd20_mode = true;
+ goto switch_fail;
}
}
}
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: add exfat filesystem code to staging
On Fri, Aug 30, 2019 at 12:04:41AM +0800, Gao Xiang wrote: > Anyway, I'm fine to delete them all if you like, but I think majority of these > are meaningful. > > data.c- /* page is already locked */ > data.c- DBG_BUGON(PageUptodate(page)); > data.c- > data.c: if (unlikely(err)) > data.c- SetPageError(page); > data.c- else > data.c- SetPageUptodate(page); If we cared about speed here then we would delete the DBG_BUGON() check because that's going to be expensive. The likely/unlikely annotations should be used in places a reasonable person thinks it will make a difference to benchmarks. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: add exfat filesystem code to staging
Hi Dan, On Fri, Aug 30, 2019 at 11:34:45AM +0300, Dan Carpenter wrote: > On Fri, Aug 30, 2019 at 12:04:41AM +0800, Gao Xiang wrote: > > Anyway, I'm fine to delete them all if you like, but I think majority of > > these > > are meaningful. > > > > data.c- /* page is already locked */ > > data.c- DBG_BUGON(PageUptodate(page)); > > data.c- > > data.c: if (unlikely(err)) > > data.c- SetPageError(page); > > data.c- else > > data.c- SetPageUptodate(page); > > If we cared about speed here then we would delete the DBG_BUGON() check > because that's going to be expensive. The likely/unlikely annotations > should be used in places a reasonable person thinks it will make a > difference to benchmarks. DBG_BUGON will be a no-op ((void)x) in non-debugging mode, I discussed related stuffs with Greg many months before [1] and other filesystems also have similar functions... p.s. I think we come to an agreement here... I killed all unlikely/likely. [1] http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128951.html sorry about no lore here. Thanks, Gao Xiang > > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/8] videodev2.h: add V4L2_DEC_CMD_FLUSH
On 8/30/19 11:38 AM, Alexandre Courbot wrote:
> On Fri, Aug 23, 2019 at 4:45 AM Jernej Skrabec
> wrote:
>>
>> From: Hans Verkuil
>>
>> Add this new V4L2_DEC_CMD_FLUSH decoder command and document it.
>>
>> Signed-off-by: Hans Verkuil
>> Signed-off-by: Jernej Skrabec
>> ---
>> Documentation/media/uapi/v4l/vidioc-decoder-cmd.rst | 11 ++-
>> Documentation/media/videodev2.h.rst.exceptions | 1 +
>> include/uapi/linux/videodev2.h | 1 +
>> 3 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/media/uapi/v4l/vidioc-decoder-cmd.rst
>> b/Documentation/media/uapi/v4l/vidioc-decoder-cmd.rst
>> index 57f0066f4cff..0bffef6058f7 100644
>> --- a/Documentation/media/uapi/v4l/vidioc-decoder-cmd.rst
>> +++ b/Documentation/media/uapi/v4l/vidioc-decoder-cmd.rst
>> @@ -208,7 +208,16 @@ introduced in Linux 3.3. They are, however, mandatory
>> for stateful mem2mem decod
>> been started yet, the driver will return an ``EPERM`` error code.
>> When
>> the decoder is already running, this command does nothing. No
>> flags are defined for this command.
>> -
>> +* - ``V4L2_DEC_CMD_FLUSH``
>> + - 4
>> + - Flush any held capture buffers. Only valid for stateless decoders,
>> +and only if ``V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF`` was set.
>> + This command is typically used when the application reached the
>> + end of the stream and the last output buffer had the
>> + ``V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF`` flag set. This would prevent
>> + dequeueing the last capture buffer containing the last decoded frame.
>> + So this command can be used to explicitly flush that last decoded
>> + frame.
>
> Just for safety, can we also specify that it is valid to call this
> command even if no buffer was held (in which case it is a no-op), as
> this can help make user-space code simpler?
Ah yes, thanks for the reminder.
Jernej, when you post the next version of this series, can you change the text
above to:
- Flush any held capture buffers. Only valid for stateless decoders.
This command is typically used when the application reached the
end of the stream and the last output buffer had the
``V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF`` flag set. This would prevent
dequeueing the capture buffer containing the last decoded frame.
So this command can be used to explicitly flush that final decoded
frame. This command does nothing if there are no held capture buffers.
Regards,
Hans
>
>>
>> Return Value
>>
>> diff --git a/Documentation/media/videodev2.h.rst.exceptions
>> b/Documentation/media/videodev2.h.rst.exceptions
>> index adeb6b7a15cb..a79028e4d929 100644
>> --- a/Documentation/media/videodev2.h.rst.exceptions
>> +++ b/Documentation/media/videodev2.h.rst.exceptions
>> @@ -434,6 +434,7 @@ replace define V4L2_DEC_CMD_START decoder-cmds
>> replace define V4L2_DEC_CMD_STOP decoder-cmds
>> replace define V4L2_DEC_CMD_PAUSE decoder-cmds
>> replace define V4L2_DEC_CMD_RESUME decoder-cmds
>> +replace define V4L2_DEC_CMD_FLUSH decoder-cmds
>>
>> replace define V4L2_DEC_CMD_START_MUTE_AUDIO decoder-cmds
>> replace define V4L2_DEC_CMD_PAUSE_TO_BLACK decoder-cmds
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 4fa9f543742d..91a79e16089c 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -1978,6 +1978,7 @@ struct v4l2_encoder_cmd {
>> #define V4L2_DEC_CMD_STOP(1)
>> #define V4L2_DEC_CMD_PAUSE (2)
>> #define V4L2_DEC_CMD_RESUME (3)
>> +#define V4L2_DEC_CMD_FLUSH (4)
>>
>> /* Flags for V4L2_DEC_CMD_START */
>> #define V4L2_DEC_CMD_START_MUTE_AUDIO (1 << 0)
>> --
>> 2.22.1
>>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/8] videodev2.h: add V4L2_DEC_CMD_FLUSH
On Fri, Aug 23, 2019 at 4:45 AM Jernej Skrabec wrote:
>
> From: Hans Verkuil
>
> Add this new V4L2_DEC_CMD_FLUSH decoder command and document it.
>
> Signed-off-by: Hans Verkuil
> Signed-off-by: Jernej Skrabec
> ---
> Documentation/media/uapi/v4l/vidioc-decoder-cmd.rst | 11 ++-
> Documentation/media/videodev2.h.rst.exceptions | 1 +
> include/uapi/linux/videodev2.h | 1 +
> 3 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/media/uapi/v4l/vidioc-decoder-cmd.rst
> b/Documentation/media/uapi/v4l/vidioc-decoder-cmd.rst
> index 57f0066f4cff..0bffef6058f7 100644
> --- a/Documentation/media/uapi/v4l/vidioc-decoder-cmd.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-decoder-cmd.rst
> @@ -208,7 +208,16 @@ introduced in Linux 3.3. They are, however, mandatory
> for stateful mem2mem decod
> been started yet, the driver will return an ``EPERM`` error code. When
> the decoder is already running, this command does nothing. No
> flags are defined for this command.
> -
> +* - ``V4L2_DEC_CMD_FLUSH``
> + - 4
> + - Flush any held capture buffers. Only valid for stateless decoders,
> +and only if ``V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF`` was set.
> + This command is typically used when the application reached the
> + end of the stream and the last output buffer had the
> + ``V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF`` flag set. This would prevent
> + dequeueing the last capture buffer containing the last decoded frame.
> + So this command can be used to explicitly flush that last decoded
> + frame.
Just for safety, can we also specify that it is valid to call this
command even if no buffer was held (in which case it is a no-op), as
this can help make user-space code simpler?
>
> Return Value
>
> diff --git a/Documentation/media/videodev2.h.rst.exceptions
> b/Documentation/media/videodev2.h.rst.exceptions
> index adeb6b7a15cb..a79028e4d929 100644
> --- a/Documentation/media/videodev2.h.rst.exceptions
> +++ b/Documentation/media/videodev2.h.rst.exceptions
> @@ -434,6 +434,7 @@ replace define V4L2_DEC_CMD_START decoder-cmds
> replace define V4L2_DEC_CMD_STOP decoder-cmds
> replace define V4L2_DEC_CMD_PAUSE decoder-cmds
> replace define V4L2_DEC_CMD_RESUME decoder-cmds
> +replace define V4L2_DEC_CMD_FLUSH decoder-cmds
>
> replace define V4L2_DEC_CMD_START_MUTE_AUDIO decoder-cmds
> replace define V4L2_DEC_CMD_PAUSE_TO_BLACK decoder-cmds
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 4fa9f543742d..91a79e16089c 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1978,6 +1978,7 @@ struct v4l2_encoder_cmd {
> #define V4L2_DEC_CMD_STOP(1)
> #define V4L2_DEC_CMD_PAUSE (2)
> #define V4L2_DEC_CMD_RESUME (3)
> +#define V4L2_DEC_CMD_FLUSH (4)
>
> /* Flags for V4L2_DEC_CMD_START */
> #define V4L2_DEC_CMD_START_MUTE_AUDIO (1 << 0)
> --
> 2.22.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/8] videodev2.h: add V4L2_DEC_CMD_FLUSH
On Fri, Aug 30, 2019 at 6:45 PM Hans Verkuil wrote:
>
> On 8/30/19 11:38 AM, Alexandre Courbot wrote:
> > On Fri, Aug 23, 2019 at 4:45 AM Jernej Skrabec
> > wrote:
> >>
> >> From: Hans Verkuil
> >>
> >> Add this new V4L2_DEC_CMD_FLUSH decoder command and document it.
> >>
> >> Signed-off-by: Hans Verkuil
> >> Signed-off-by: Jernej Skrabec
> >> ---
> >> Documentation/media/uapi/v4l/vidioc-decoder-cmd.rst | 11 ++-
> >> Documentation/media/videodev2.h.rst.exceptions | 1 +
> >> include/uapi/linux/videodev2.h | 1 +
> >> 3 files changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/media/uapi/v4l/vidioc-decoder-cmd.rst
> >> b/Documentation/media/uapi/v4l/vidioc-decoder-cmd.rst
> >> index 57f0066f4cff..0bffef6058f7 100644
> >> --- a/Documentation/media/uapi/v4l/vidioc-decoder-cmd.rst
> >> +++ b/Documentation/media/uapi/v4l/vidioc-decoder-cmd.rst
> >> @@ -208,7 +208,16 @@ introduced in Linux 3.3. They are, however, mandatory
> >> for stateful mem2mem decod
> >> been started yet, the driver will return an ``EPERM`` error code.
> >> When
> >> the decoder is already running, this command does nothing. No
> >> flags are defined for this command.
> >> -
> >> +* - ``V4L2_DEC_CMD_FLUSH``
> >> + - 4
> >> + - Flush any held capture buffers. Only valid for stateless decoders,
> >> +and only if ``V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF`` was
> >> set.
> >> + This command is typically used when the application reached the
> >> + end of the stream and the last output buffer had the
> >> + ``V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF`` flag set. This would prevent
> >> + dequeueing the last capture buffer containing the last decoded
> >> frame.
> >> + So this command can be used to explicitly flush that last decoded
> >> + frame.
> >
> > Just for safety, can we also specify that it is valid to call this
> > command even if no buffer was held (in which case it is a no-op), as
> > this can help make user-space code simpler?
>
> Ah yes, thanks for the reminder.
>
> Jernej, when you post the next version of this series, can you change the text
> above to:
>
> - Flush any held capture buffers. Only valid for stateless decoders.
> This command is typically used when the application reached the
> end of the stream and the last output buffer had the
> ``V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF`` flag set. This would prevent
> dequeueing the capture buffer containing the last decoded frame.
> So this command can be used to explicitly flush that final decoded
> frame. This command does nothing if there are no held capture buffers.
With the above,
Reviewed-by: Alexandre Courbot
Thanks!
>
> Regards,
>
> Hans
>
> >
> >>
> >> Return Value
> >>
> >> diff --git a/Documentation/media/videodev2.h.rst.exceptions
> >> b/Documentation/media/videodev2.h.rst.exceptions
> >> index adeb6b7a15cb..a79028e4d929 100644
> >> --- a/Documentation/media/videodev2.h.rst.exceptions
> >> +++ b/Documentation/media/videodev2.h.rst.exceptions
> >> @@ -434,6 +434,7 @@ replace define V4L2_DEC_CMD_START decoder-cmds
> >> replace define V4L2_DEC_CMD_STOP decoder-cmds
> >> replace define V4L2_DEC_CMD_PAUSE decoder-cmds
> >> replace define V4L2_DEC_CMD_RESUME decoder-cmds
> >> +replace define V4L2_DEC_CMD_FLUSH decoder-cmds
> >>
> >> replace define V4L2_DEC_CMD_START_MUTE_AUDIO decoder-cmds
> >> replace define V4L2_DEC_CMD_PAUSE_TO_BLACK decoder-cmds
> >> diff --git a/include/uapi/linux/videodev2.h
> >> b/include/uapi/linux/videodev2.h
> >> index 4fa9f543742d..91a79e16089c 100644
> >> --- a/include/uapi/linux/videodev2.h
> >> +++ b/include/uapi/linux/videodev2.h
> >> @@ -1978,6 +1978,7 @@ struct v4l2_encoder_cmd {
> >> #define V4L2_DEC_CMD_STOP(1)
> >> #define V4L2_DEC_CMD_PAUSE (2)
> >> #define V4L2_DEC_CMD_RESUME (3)
> >> +#define V4L2_DEC_CMD_FLUSH (4)
> >>
> >> /* Flags for V4L2_DEC_CMD_START */
> >> #define V4L2_DEC_CMD_START_MUTE_AUDIO (1 << 0)
> >> --
> >> 2.22.1
> >>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] erofs: using switch-case while checking the inode type.
while filling the linux inode, using switch-case statement to check
the type of inode.
switch-case statement looks more clean here.
Signed-off-by: Pratik Shinde
---
fs/erofs/inode.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
index 80f4fe9..6336cc1 100644
--- a/fs/erofs/inode.c
+++ b/fs/erofs/inode.c
@@ -190,22 +190,28 @@ static int fill_inode(struct inode *inode, int isdir)
err = read_inode(inode, data + ofs);
if (!err) {
/* setup the new inode */
- if (S_ISREG(inode->i_mode)) {
+ switch (inode->i_mode & S_IFMT) {
+ case S_IFREG:
inode->i_op = &erofs_generic_iops;
inode->i_fop = &generic_ro_fops;
- } else if (S_ISDIR(inode->i_mode)) {
+ break;
+ case S_IFDIR:
inode->i_op = &erofs_dir_iops;
inode->i_fop = &erofs_dir_fops;
- } else if (S_ISLNK(inode->i_mode)) {
+ break;
+ case S_IFLNK:
/* by default, page_get_link is used for symlink */
inode->i_op = &erofs_symlink_iops;
inode_nohighmem(inode);
- } else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) ||
- S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) {
+ break;
+ case S_IFCHR:
+ case S_IFBLK:
+ case S_IFIFO:
+ case S_IFSOCK:
inode->i_op = &erofs_generic_iops;
init_special_inode(inode, inode->i_mode, inode->i_rdev);
goto out_unlock;
- } else {
+ default:
err = -EFSCORRUPTED;
goto out_unlock;
}
--
2.9.3
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rts5208: Fixed checkpath warning.
On Fri, 2019-08-30 at 12:41 +0530, Prakhar Sinha wrote: > This patch solves the following checkpatch.pl's message in > drivers/staging/rts5208/rtsx_transport.c:397. > WARNING: line over 80 characters > + option = RTSX_SG_VALID | RTSX_SG_END | > RTSX_SG_TRANS_DATA; [] > diff --git a/drivers/staging/rts5208/rtsx_transport.c > b/drivers/staging/rts5208/rtsx_transport.c [] > @@ -394,7 +394,8 @@ static int rtsx_transfer_sglist_adma_partial(struct > rtsx_chip *chip, u8 card, > *index = *index + 1; > } > if ((i == (sg_cnt - 1)) || !resid) > - option = RTSX_SG_VALID | RTSX_SG_END | > RTSX_SG_TRANS_DATA; > + option = RTSX_SG_VALID | RTSX_SG_END | > + RTSX_SG_TRANS_DATA; > else > option = RTSX_SG_VALID | RTSX_SG_TRANS_DATA; probably more readable as: option = RTXS_SG_VALID | RTSX_SG_TRANS_DATA; if (i == sg_cnt - 1 || !resid) option |= RTXS_SG_END; The compiler should produce the same object code. Add parentheses in the if to suit, but they are not necessary. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: add exfat filesystem code to staging
On Fri, Aug 30, 2019 at 04:43:33PM +0800, Gao Xiang wrote: > Hi Dan, > > On Fri, Aug 30, 2019 at 11:34:45AM +0300, Dan Carpenter wrote: > > On Fri, Aug 30, 2019 at 12:04:41AM +0800, Gao Xiang wrote: > > > Anyway, I'm fine to delete them all if you like, but I think majority of > > > these > > > are meaningful. > > > > > > data.c- /* page is already locked */ > > > data.c- DBG_BUGON(PageUptodate(page)); > > > data.c- > > > data.c: if (unlikely(err)) > > > data.c- SetPageError(page); > > > data.c- else > > > data.c- SetPageUptodate(page); > > > > If we cared about speed here then we would delete the DBG_BUGON() check > > because that's going to be expensive. The likely/unlikely annotations > > should be used in places a reasonable person thinks it will make a > > difference to benchmarks. > > DBG_BUGON will be a no-op ((void)x) in non-debugging mode, It expands to: ((void)PageUptodate(page)); Calling PageUptodate() doesn't do anything, but it isn't free. The time it takes to do that function call completely negates any speed up from using likely/unlikely. I'm really not trying to be a jerk... regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
On Fri, Aug 30, 2019 at 11:36:42AM +0800, Gao Xiang wrote: > As Dan Carpenter suggested [1], I have to remove > all erofs likely/unlikely annotations. > > [1] https://lore.kernel.org/linux-fsdevel/20190829154346.GK23584@kadam/ > Reported-by: Dan Carpenter > Signed-off-by: Gao Xiang > --- Thanks! This is a nice readability improvement and I'm so sure it won't impact benchmarking at all. Acked-by: Dan Carpenter regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/4] binder: Add stats, state and transactions files
On Thu, Aug 29, 2019 at 02:18:10PM -0700, Hridya Valsaraju wrote:
> The following binder stat files currently live in debugfs.
>
> /sys/kernel/debug/binder/state
> /sys/kernel/debug/binder/stats
> /sys/kernel/debug/binder/transactions
>
> This patch makes these files available in a binderfs instance
> mounted with the mount option 'stats=global'. For example, if a binderfs
> instance is mounted at path /dev/binderfs, the above files will be
> available at the following locations:
>
> /dev/binderfs/binder_logs/state
> /dev/binderfs/binder_logs/stats
> /dev/binderfs/binder_logs/transactions
>
> This provides a way to access them even when debugfs is not mounted.
>
> Signed-off-by: Hridya Valsaraju
Just two comments below. If you have addressed them you can add my:
Acked-by: Christian Brauner
> ---
>
> Changes in v2:
> - Consistently name variables across functions as per Christian
>Brauner.
> - Improve check for binderfs device in binderfs_evict_inode()
>as per Christian Brauner.
>
> drivers/android/binder.c | 15 ++--
> drivers/android/binder_internal.h | 8 ++
> drivers/android/binderfs.c| 140 +-
> 3 files changed, 153 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index ca6b21a53321..de795bd229c4 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -6055,7 +6055,7 @@ static void print_binder_proc_stats(struct seq_file *m,
> }
>
>
> -static int state_show(struct seq_file *m, void *unused)
> +int binder_state_show(struct seq_file *m, void *unused)
> {
> struct binder_proc *proc;
> struct binder_node *node;
> @@ -6094,7 +6094,7 @@ static int state_show(struct seq_file *m, void *unused)
> return 0;
> }
>
> -static int stats_show(struct seq_file *m, void *unused)
> +int binder_stats_show(struct seq_file *m, void *unused)
> {
> struct binder_proc *proc;
>
> @@ -6110,7 +6110,7 @@ static int stats_show(struct seq_file *m, void *unused)
> return 0;
> }
>
> -static int transactions_show(struct seq_file *m, void *unused)
> +int binder_transactions_show(struct seq_file *m, void *unused)
> {
> struct binder_proc *proc;
>
> @@ -6198,9 +6198,6 @@ const struct file_operations binder_fops = {
> .release = binder_release,
> };
>
> -DEFINE_SHOW_ATTRIBUTE(state);
> -DEFINE_SHOW_ATTRIBUTE(stats);
> -DEFINE_SHOW_ATTRIBUTE(transactions);
> DEFINE_SHOW_ATTRIBUTE(transaction_log);
>
> static int __init init_binder_device(const char *name)
> @@ -6256,17 +6253,17 @@ static int __init binder_init(void)
> 0444,
> binder_debugfs_dir_entry_root,
> NULL,
> - &state_fops);
> + &binder_state_fops);
> debugfs_create_file("stats",
> 0444,
> binder_debugfs_dir_entry_root,
> NULL,
> - &stats_fops);
> + &binder_stats_fops);
> debugfs_create_file("transactions",
> 0444,
> binder_debugfs_dir_entry_root,
> NULL,
> - &transactions_fops);
> + &binder_transactions_fops);
> debugfs_create_file("transaction_log",
> 0444,
> binder_debugfs_dir_entry_root,
> diff --git a/drivers/android/binder_internal.h
> b/drivers/android/binder_internal.h
> index fe8c745dc8e0..12ef96f256c6 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -57,4 +57,12 @@ static inline int __init init_binderfs(void)
> }
> #endif
>
> +int binder_stats_show(struct seq_file *m, void *unused);
> +DEFINE_SHOW_ATTRIBUTE(binder_stats);
> +
> +int binder_state_show(struct seq_file *m, void *unused);
> +DEFINE_SHOW_ATTRIBUTE(binder_state);
> +
> +int binder_transactions_show(struct seq_file *m, void *unused);
> +DEFINE_SHOW_ATTRIBUTE(binder_transactions);
> #endif /* _LINUX_BINDER_INTERNAL_H */
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index 7045bfe5b52b..0e1e7c87cd33 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -280,7 +280,7 @@ static void binderfs_evict_inode(struct inode *inode)
>
> clear_inode(inode);
>
> - if (!device)
> + if (!S_ISCHR(inode->i_mode) || !device)
> return;
>
> mutex_lock(&binderfs_minors_mutex);
> @@ -502,6 +502,141 @@ static const struct inode_operations
> binderfs_dir_inode_operations = {
> .unlink = binderfs_unlink,
> };
>
> +static struct inode *binderfs_make_inode(struct super_block *sb, i
Re: [PATCH v2 3/4] binder: Make transaction_log available in binderfs
On Thu, Aug 29, 2019 at 02:18:11PM -0700, Hridya Valsaraju wrote:
> Currently, the binder transaction log files 'transaction_log'
> and 'failed_transaction_log' live in debugfs at the following locations:
>
> /sys/kernel/debug/binder/failed_transaction_log
> /sys/kernel/debug/binder/transaction_log
>
> This patch makes these files also available in a binderfs instance
> mounted with the mount option "stats=global".
> It does not affect the presence of these files in debugfs.
> If a binderfs instance is mounted at path /dev/binderfs, the location of
> these files will be as follows:
>
> /dev/binderfs/binder_logs/failed_transaction_log
> /dev/binderfs/binder_logs/transaction_log
>
> This change provides an alternate option to access these files when
> debugfs is not mounted.
>
> Signed-off-by: Hridya Valsaraju
(If you don't change this patch in the next version, please just keep my:
Acked-by: Christian Brauner
when sending it out. :)
> ---
>
> Changes in v2:
> -Consistent variable naming accross functions as per Christian Brauner.
>
> drivers/android/binder.c | 34 +--
> drivers/android/binder_internal.h | 30 +++
> drivers/android/binderfs.c| 18
> 3 files changed, 53 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index de795bd229c4..bed217310197 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -197,30 +197,8 @@ static inline void binder_stats_created(enum
> binder_stat_types type)
> atomic_inc(&binder_stats.obj_created[type]);
> }
>
> -struct binder_transaction_log_entry {
> - int debug_id;
> - int debug_id_done;
> - int call_type;
> - int from_proc;
> - int from_thread;
> - int target_handle;
> - int to_proc;
> - int to_thread;
> - int to_node;
> - int data_size;
> - int offsets_size;
> - int return_error_line;
> - uint32_t return_error;
> - uint32_t return_error_param;
> - const char *context_name;
> -};
> -struct binder_transaction_log {
> - atomic_t cur;
> - bool full;
> - struct binder_transaction_log_entry entry[32];
> -};
> -static struct binder_transaction_log binder_transaction_log;
> -static struct binder_transaction_log binder_transaction_log_failed;
> +struct binder_transaction_log binder_transaction_log;
> +struct binder_transaction_log binder_transaction_log_failed;
>
> static struct binder_transaction_log_entry *binder_transaction_log_add(
> struct binder_transaction_log *log)
> @@ -6166,7 +6144,7 @@ static void print_binder_transaction_log_entry(struct
> seq_file *m,
> "\n" : " (incomplete)\n");
> }
>
> -static int transaction_log_show(struct seq_file *m, void *unused)
> +int binder_transaction_log_show(struct seq_file *m, void *unused)
> {
> struct binder_transaction_log *log = m->private;
> unsigned int log_cur = atomic_read(&log->cur);
> @@ -6198,8 +6176,6 @@ const struct file_operations binder_fops = {
> .release = binder_release,
> };
>
> -DEFINE_SHOW_ATTRIBUTE(transaction_log);
> -
> static int __init init_binder_device(const char *name)
> {
> int ret;
> @@ -6268,12 +6244,12 @@ static int __init binder_init(void)
> 0444,
> binder_debugfs_dir_entry_root,
> &binder_transaction_log,
> - &transaction_log_fops);
> + &binder_transaction_log_fops);
> debugfs_create_file("failed_transaction_log",
> 0444,
> binder_debugfs_dir_entry_root,
> &binder_transaction_log_failed,
> - &transaction_log_fops);
> + &binder_transaction_log_fops);
> }
>
> if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
> diff --git a/drivers/android/binder_internal.h
> b/drivers/android/binder_internal.h
> index 12ef96f256c6..b9be42d9464c 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -65,4 +65,34 @@ DEFINE_SHOW_ATTRIBUTE(binder_state);
>
> int binder_transactions_show(struct seq_file *m, void *unused);
> DEFINE_SHOW_ATTRIBUTE(binder_transactions);
> +
> +int binder_transaction_log_show(struct seq_file *m, void *unused);
> +DEFINE_SHOW_ATTRIBUTE(binder_transaction_log);
> +
> +struct binder_transaction_log_entry {
> + int debug_id;
> + int debug_id_done;
> + int call_type;
> + int from_proc;
> + int from_thread;
> + int target_handle;
> + int to_proc;
> + int to_thread;
> + int to_node;
> + int data_size;
> + int offsets_size;
> + int return_error_line;
> + uint32_t return_error;
> + uint32_t return_error_param;
> + const char *context_
Re: [PATCH v2 4/4] binder: Add binder_proc logging to binderfs
On Thu, Aug 29, 2019 at 02:18:12PM -0700, Hridya Valsaraju wrote:
> Currently /sys/kernel/debug/binder/proc contains
> the debug data for every binder_proc instance.
> This patch makes this information also available
> in a binderfs instance mounted with a mount option
> "stats=global" in addition to debugfs. The patch does
> not affect the presence of the file in debugfs.
>
> If a binderfs instance is mounted at path /dev/binderfs,
> this file would be present at /dev/binderfs/binder_logs/proc.
> This change provides an alternate way to access this file when debugfs
> is not mounted.
>
> Signed-off-by: Hridya Valsaraju
Same as for the previous patch: Please keep my Acked-by if you don't
change this patch when you send out a new version.
Acked-by: Christian Brauner
> ---
>
> Changes in v2:
> - Consistent variable naming across functions as per Christian
> Brauner.
> - As suggested by Todd Kjos, log a failure to create a
>process-specific binderfs log file if the error code is not EEXIST
>since an error code of EEXIST is expected if
>multiple contexts of the same process try to create the file during
>binder_open().
>
> drivers/android/binder.c | 46 -
> drivers/android/binder_internal.h | 46 +
> drivers/android/binderfs.c| 68 ++-
> 3 files changed, 121 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index bed217310197..ee610ea48309 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -481,6 +481,7 @@ struct binder_priority {
> * @inner_lock: can nest under outer_lock and/or node lock
> * @outer_lock: no nesting under innor or node lock
> *Lock order: 1) outer, 2) node, 3) inner
> + * @binderfs_entry: process-specific binderfs log file
> *
> * Bookkeeping structure for binder processes
> */
> @@ -510,6 +511,7 @@ struct binder_proc {
> struct binder_context *context;
> spinlock_t inner_lock;
> spinlock_t outer_lock;
> + struct dentry *binderfs_entry;
> };
>
> enum {
> @@ -5347,6 +5349,8 @@ static int binder_open(struct inode *nodp, struct file
> *filp)
> {
> struct binder_proc *proc;
> struct binder_device *binder_dev;
> + struct binderfs_info *info;
> + struct dentry *binder_binderfs_dir_entry_proc = NULL;
>
> binder_debug(BINDER_DEBUG_OPEN_CLOSE, "%s: %d:%d\n", __func__,
>current->group_leader->pid, current->pid);
> @@ -5368,11 +5372,14 @@ static int binder_open(struct inode *nodp, struct
> file *filp)
> }
>
> /* binderfs stashes devices in i_private */
> - if (is_binderfs_device(nodp))
> + if (is_binderfs_device(nodp)) {
> binder_dev = nodp->i_private;
> - else
> + info = nodp->i_sb->s_fs_info;
> + binder_binderfs_dir_entry_proc = info->proc_log_dir;
> + } else {
> binder_dev = container_of(filp->private_data,
> struct binder_device, miscdev);
> + }
> proc->context = &binder_dev->context;
> binder_alloc_init(&proc->alloc);
>
> @@ -5403,6 +5410,35 @@ static int binder_open(struct inode *nodp, struct file
> *filp)
> &proc_fops);
> }
>
> + if (binder_binderfs_dir_entry_proc) {
> + char strbuf[11];
> + struct dentry *binderfs_entry;
> +
> + snprintf(strbuf, sizeof(strbuf), "%u", proc->pid);
> + /*
> + * Similar to debugfs, the process specific log file is shared
> + * between contexts. If the file has already been created for a
> + * process, the following binderfs_create_file() call will
> + * fail with error code EEXIST if another context of the same
> + * process invoked binder_open(). This is ok since same as
> + * debugfs, the log file will contain information on all
> + * contexts of a given PID.
> + */
> + binderfs_entry =
> binderfs_create_file(binder_binderfs_dir_entry_proc,
> + strbuf, &proc_fops, (void *)(unsigned long)proc->pid);
> + if (!IS_ERR(binderfs_entry)) {
> + proc->binderfs_entry = binderfs_entry;
> + } else {
> + int error;
> +
> + error = PTR_ERR(binderfs_entry);
> + if (error != -EEXIST) {
> + pr_warn("Unable to create file %s in binderfs
> (error %d)\n",
> + strbuf, error);
> + }
> + }
> + }
> +
> return 0;
> }
>
> @@ -5442,6 +5478,12 @@ static int binder_release(struct inode *nodp, struct
> file *filp)
> struct binder_proc *proc = filp->private_data;
>
> debugfs_remove(proc->d
Re: [PATCH] staging: exfat: add exfat filesystem code to staging
On Fri, Aug 30, 2019 at 10:06:25AM +0800, Chao Yu wrote: > On 2019/8/29 23:43, Dan Carpenter wrote: > >> p.s. There are 2947 (un)likely places in fs/ directory. > > > > I was complaining about you adding new pointless ones, not existing > > ones. The likely/unlikely annotations are supposed to be functional and > > not decorative. I explained this very clearly. > > > > Probably most of the annotations in fs/ are wrong but they are also > > harmless except for the slight messiness. However there are definitely > > some which are important so removing them all isn't a good idea. > > Hi Dan, > > Could you please pick up one positive example using likely and unlikely > correctly? so we can follow the example, rather than removing them all > blindly. Remove all of them and re-add with explanation if and how each is going to make things better. If you can't reason about, prove by benchmarks or point to inefficient asm code generated, then don't add them again. The feedback I got from CPU and compiler people over the years is not to bother using the annotations. CPUs are doing dynamic branch prediction and compilers are applying tons of optimizations. GCC docs state about the builtin: "In general, you should prefer to use actual profile feedback for this (-fprofile-arcs), as programmers are notoriously bad at predicting how their programs actually perform." (https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html) ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
On Fri, Aug 30, 2019 at 02:25:13PM +0800, Chao Yu wrote: > On 2019/8/30 11:36, Gao Xiang wrote: > > As Dan Carpenter suggested [1], I have to remove > > all erofs likely/unlikely annotations. > > > > [1] https://lore.kernel.org/linux-fsdevel/20190829154346.GK23584@kadam/ > > Reported-by: Dan Carpenter > > Signed-off-by: Gao Xiang > > I suggest we can modify this by following good example rather than removing > them > all, at least, the fuzzed random fields of disk layout handling should be very > rare case, I guess it's fine to use unlikely. No, no... It's the opposite. Only use those annotations on fast paths where it's going to show up in benchmarks. On fast paths then remove all the debug code and really optimize the heck out of the code. We sacrifice readability for speed in places where it matters. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: using switch-case while checking the inode type.
Hi Pratik,
The subject line could be better as '[PATCH v2] xx'...
On Fri, Aug 30, 2019 at 03:26:15PM +0530, Pratik Shinde wrote:
> while filling the linux inode, using switch-case statement to check
> the type of inode.
> switch-case statement looks more clean here.
>
> Signed-off-by: Pratik Shinde
I have no problem of this patch, and I will do a test and reply
you "Reviewed-by:" in hours (I'm doing another patchset to resolve
what Christoph suggested again)...
Thanks,
Gao Xiang
> ---
> fs/erofs/inode.c | 18 --
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> index 80f4fe9..6336cc1 100644
> --- a/fs/erofs/inode.c
> +++ b/fs/erofs/inode.c
> @@ -190,22 +190,28 @@ static int fill_inode(struct inode *inode, int isdir)
> err = read_inode(inode, data + ofs);
> if (!err) {
> /* setup the new inode */
> - if (S_ISREG(inode->i_mode)) {
> + switch (inode->i_mode & S_IFMT) {
> + case S_IFREG:
> inode->i_op = &erofs_generic_iops;
> inode->i_fop = &generic_ro_fops;
> - } else if (S_ISDIR(inode->i_mode)) {
> + break;
> + case S_IFDIR:
> inode->i_op = &erofs_dir_iops;
> inode->i_fop = &erofs_dir_fops;
> - } else if (S_ISLNK(inode->i_mode)) {
> + break;
> + case S_IFLNK:
> /* by default, page_get_link is used for symlink */
> inode->i_op = &erofs_symlink_iops;
> inode_nohighmem(inode);
> - } else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) ||
> - S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) {
> + break;
> + case S_IFCHR:
> + case S_IFBLK:
> + case S_IFIFO:
> + case S_IFSOCK:
> inode->i_op = &erofs_generic_iops;
> init_special_inode(inode, inode->i_mode, inode->i_rdev);
> goto out_unlock;
> - } else {
> + default:
> err = -EFSCORRUPTED;
> goto out_unlock;
> }
> --
> 2.9.3
>
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Checking usage of likeliness annotations
> I'm also curious about that, what is the filesystem or kernel standard about > likely/unlikely use (since I didn't find some documented standard > so I used in my personal way, Such information is helpful. > I think it is reasonable at least to cover all error handling paths), I hope so, too. I imagine that the likeliness annotation usage could depend also on software build parameters. Would you occasionally like to influence corresponding probabilities any more by system settings? > maybe I'm an _idiot_ I hope not. > as some earlier unfriendly word said somewhere > so I'm too stupid to understand the implicit meaning of some document. Can the software development discussion be continued in more constructive ways? Regards, Markus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: add exfat filesystem code to staging
Hi Dan, On Fri, Aug 30, 2019 at 02:26:12PM +0300, Dan Carpenter wrote: > On Fri, Aug 30, 2019 at 04:43:33PM +0800, Gao Xiang wrote: > > Hi Dan, > > > > On Fri, Aug 30, 2019 at 11:34:45AM +0300, Dan Carpenter wrote: > > > On Fri, Aug 30, 2019 at 12:04:41AM +0800, Gao Xiang wrote: > > > > Anyway, I'm fine to delete them all if you like, but I think majority > > > > of these > > > > are meaningful. > > > > > > > > data.c- /* page is already locked */ > > > > data.c- DBG_BUGON(PageUptodate(page)); > > > > data.c- > > > > data.c: if (unlikely(err)) > > > > data.c- SetPageError(page); > > > > data.c- else > > > > data.c- SetPageUptodate(page); > > > > > > If we cared about speed here then we would delete the DBG_BUGON() check > > > because that's going to be expensive. The likely/unlikely annotations > > > should be used in places a reasonable person thinks it will make a > > > difference to benchmarks. > > > > DBG_BUGON will be a no-op ((void)x) in non-debugging mode, > > It expands to: > > ((void)PageUptodate(page)); > > Calling PageUptodate() doesn't do anything, but it isn't free. The > time it takes to do that function call completely negates any speed up > from using likely/unlikely. > > I'm really not trying to be a jerk... You are right, I recalled that PageUptodate is not as simple as it implys. Yes, those are all removed now... I am ok with that, thanks for your suggestion :) Thanks, Gao Xiang > > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
Hi Dan, On Fri, Aug 30, 2019 at 02:30:47PM +0300, Dan Carpenter wrote: > On Fri, Aug 30, 2019 at 11:36:42AM +0800, Gao Xiang wrote: > > As Dan Carpenter suggested [1], I have to remove > > all erofs likely/unlikely annotations. > > > > [1] https://lore.kernel.org/linux-fsdevel/20190829154346.GK23584@kadam/ > > Reported-by: Dan Carpenter > > Signed-off-by: Gao Xiang > > --- > > Thanks! > > This is a nice readability improvement and I'm so sure it won't impact > benchmarking at all. > > Acked-by: Dan Carpenter It seems Greg merged another version... I have no idea but thanks for your acked-by :) https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=8d8a09b093d7073465c824f74caf315c073d3875 THanks, Gao Xiang > > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 01/24] erofs: add on-disk layout
On Thu, Aug 29, 2019 at 08:58:17AM -0700, Joe Perches wrote:
> On Thu, 2019-08-29 at 18:32 +0800, Gao Xiang wrote:
> > Hi Christoph,
> >
> > On Thu, Aug 29, 2019 at 02:59:54AM -0700, Christoph Hellwig wrote:
> > > > --- /dev/null
> > > > +++ b/fs/erofs/erofs_fs.h
> > > > @@ -0,0 +1,316 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only OR Apache-2.0 */
> > > > +/*
> > > > + * linux/fs/erofs/erofs_fs.h
> > >
> > > Please remove the pointless file names in the comment headers.
> >
> > Already removed in the latest version.
> >
> > > > +struct erofs_super_block {
> > > > +/* 0 */__le32 magic; /* in the little endian */
> > > > +/* 4 */__le32 checksum;/* crc32c(super_block) */
> > > > +/* 8 */__le32 features;/* (aka. feature_compat) */
> > > > +/* 12 */__u8 blkszbits; /* support block_size == PAGE_SIZE
> > > > only */
> > >
> > > Please remove all the byte offset comments. That is something that can
> > > easily be checked with gdb or pahole.
> >
> > I have no idea the actual issue here.
> > It will help all developpers better add fields or calculate
> > these offsets in their mind, and with care.
> >
> > Rather than they didn't run "gdb" or "pahole" and change it by mistake.
>
> I think Christoph is not right here.
>
> Using external tools for validation is extra work
> when necessary for understanding the code.
The advantage of using the external tools that the information about
offsets is provably correct ...
> The expected offset is somewhat valuable, but
> perhaps the form is a bit off given the visual
> run-in to the field types.
>
> The extra work with this form is manipulating all
> the offsets whenever a structure change occurs.
... while this is error prone.
> The comments might be better with a form more like:
>
> struct erofs_super_block {/* offset description */
> __le32 magic; /* 0 */
> __le32 checksum;/* 4 crc32c(super_block) */
> __le32 features;/* 8 (aka. feature_compat) */
> __u8 blkszbits; /* 12 support block_size == PAGE_SIZE only */
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: Use kmem_cache for binder_thread
On Fri, Aug 30, 2019 at 08:38:51AM +0200, Greg KH wrote: > On Thu, Aug 29, 2019 at 05:27:22PM +0200, Christian Brauner wrote: > > On Thu, Aug 29, 2019 at 09:53:59AM -0400, Joel Fernandes wrote: > > > On Thu, Aug 29, 2019 at 08:42:29AM +0200, Greg KH wrote: > > > > On Thu, Aug 29, 2019 at 01:49:53PM +0800, Peikan Tsai wrote: > > > [snip] > > > > > The allocated size for each binder_thread is 512 bytes by kzalloc. > > > > > Because the size of binder_thread is fixed and it's only 304 bytes. > > > > > It will save 208 bytes per binder_thread when use create a kmem_cache > > > > > for the binder_thread. > > > > > > > > Are you _sure_ it really will save that much memory? You want to do > > > > allocations based on a nice alignment for lots of good reasons, > > > > especially for something that needs quick accesses. > > > > > > Alignment can be done for slab allocations, kmem_cache_create() takes an > > > align argument. I am not sure what the default alignment of objects is > > > though (probably no default alignment). What is an optimal alignment in > > > your > > > view? > > > > Probably SLAB_HWCACHE_ALIGN would make most sense. > > This isn't memory accessing hardware, so I don't think it would, right? I was more thinking of cacheline bouncing under contention. But maybe that's not worth it in this case... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: rts5208: Fixed checkpath warning.
This patch solves the following checkpatch.pl's message in drivers/staging/rts5208/rtsx_transport.c:397. WARNING: line over 80 characters + option = RTSX_SG_VALID | RTSX_SG_END | RTSX_SG_TRANS_DATA; Signed-off-by: Prakhar Sinha --- Changes in v2: - Re-structured assignment to solve checkpath.pl's warning. drivers/staging/rts5208/rtsx_transport.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/staging/rts5208/rtsx_transport.c b/drivers/staging/rts5208/rtsx_transport.c index 8277d7895608..e61bc7c6ac33 100644 --- a/drivers/staging/rts5208/rtsx_transport.c +++ b/drivers/staging/rts5208/rtsx_transport.c @@ -393,10 +393,9 @@ static int rtsx_transfer_sglist_adma_partial(struct rtsx_chip *chip, u8 card, *offset = 0; *index = *index + 1; } - if ((i == (sg_cnt - 1)) || !resid) - option = RTSX_SG_VALID | RTSX_SG_END | RTSX_SG_TRANS_DATA; - else - option = RTSX_SG_VALID | RTSX_SG_TRANS_DATA; + option = RTSX_SG_VALID | RTSX_SG_TRANS_DATA; + if ((i == sg_cnt - 1) || !resid) + option |= RTSX_SG_END; rtsx_add_sg_tbl(chip, (u32)addr, (u32)len, option); -- 2.20.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: rts5208: Fix checkpath warning
This patch fixes the following checkpath warning in the file drivers/staging/rts5208/rtsx_transport.c:546 WARNING: line over 80 characters + option = RTSX_SG_VALID | RTSX_SG_END | RTSX_SG_TRANS_DATA; Signed-off-by: P SAI PRASANTH --- Changes in v2: -restructured code for better fixing the checkpath warning -wrapped commit description drivers/staging/rts5208/rtsx_transport.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/staging/rts5208/rtsx_transport.c b/drivers/staging/rts5208/rtsx_transport.c index 8277d78..48c782f 100644 --- a/drivers/staging/rts5208/rtsx_transport.c +++ b/drivers/staging/rts5208/rtsx_transport.c @@ -540,11 +540,10 @@ static int rtsx_transfer_sglist_adma(struct rtsx_chip *chip, u8 card, dev_dbg(rtsx_dev(chip), "DMA addr: 0x%x, Len: 0x%x\n", (unsigned int)addr, len); - + + option = RTSX_SG_VALID | RTSX_SG_TRANS_DATA; if (j == (sg_cnt - 1)) - option = RTSX_SG_VALID | RTSX_SG_END | RTSX_SG_TRANS_DATA; - else - option = RTSX_SG_VALID | RTSX_SG_TRANS_DATA; + option |= RTSX_SG_END rtsx_add_sg_tbl(chip, (u32)addr, (u32)len, option); -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 01/24] erofs: add on-disk layout
Hi David,
On Fri, Aug 30, 2019 at 02:07:14PM +0200, David Sterba wrote:
> On Thu, Aug 29, 2019 at 08:58:17AM -0700, Joe Perches wrote:
> > On Thu, 2019-08-29 at 18:32 +0800, Gao Xiang wrote:
> > > Hi Christoph,
> > >
> > > On Thu, Aug 29, 2019 at 02:59:54AM -0700, Christoph Hellwig wrote:
> > > > > --- /dev/null
> > > > > +++ b/fs/erofs/erofs_fs.h
> > > > > @@ -0,0 +1,316 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-only OR Apache-2.0 */
> > > > > +/*
> > > > > + * linux/fs/erofs/erofs_fs.h
> > > >
> > > > Please remove the pointless file names in the comment headers.
> > >
> > > Already removed in the latest version.
> > >
> > > > > +struct erofs_super_block {
> > > > > +/* 0 */__le32 magic; /* in the little endian */
> > > > > +/* 4 */__le32 checksum;/* crc32c(super_block) */
> > > > > +/* 8 */__le32 features;/* (aka. feature_compat) */
> > > > > +/* 12 */__u8 blkszbits; /* support block_size == PAGE_SIZE
> > > > > only */
> > > >
> > > > Please remove all the byte offset comments. That is something that can
> > > > easily be checked with gdb or pahole.
> > >
> > > I have no idea the actual issue here.
> > > It will help all developpers better add fields or calculate
> > > these offsets in their mind, and with care.
> > >
> > > Rather than they didn't run "gdb" or "pahole" and change it by mistake.
> >
> > I think Christoph is not right here.
> >
> > Using external tools for validation is extra work
> > when necessary for understanding the code.
>
> The advantage of using the external tools that the information about
> offsets is provably correct ...
>
> > The expected offset is somewhat valuable, but
> > perhaps the form is a bit off given the visual
> > run-in to the field types.
> >
> > The extra work with this form is manipulating all
> > the offsets whenever a structure change occurs.
>
> ... while this is error prone.
I will redo a full patchset and comments addressing
what Christoph all said yesterday.
Either form is fine with me for this case, let's remove
them instead.
Thanks,
Gao Xiang
>
> > The comments might be better with a form more like:
> >
> > struct erofs_super_block { /* offset description */
> > __le32 magic; /* 0 */
> > __le32 checksum;/* 4 crc32c(super_block) */
> > __le32 features;/* 8 (aka. feature_compat) */
> > __u8 blkszbits; /* 12 support block_size == PAGE_SIZE only */
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] erofs: using switch-case while checking the inode type.
On Fri, Aug 30, 2019 at 07:59:48PM +0800, Gao Xiang wrote:
> Hi Pratik,
>
> The subject line could be better as '[PATCH v2] xx'...
>
> On Fri, Aug 30, 2019 at 03:26:15PM +0530, Pratik Shinde wrote:
> > while filling the linux inode, using switch-case statement to check
> > the type of inode.
> > switch-case statement looks more clean here.
> >
> > Signed-off-by: Pratik Shinde
>
> I have no problem of this patch, and I will do a test and reply
> you "Reviewed-by:" in hours (I'm doing another patchset to resolve
> what Christoph suggested again)...
Reviewed-by: Gao Xiang
Thanks,
Gao Xiang
>
> Thanks,
> Gao Xiang
>
> > ---
> > fs/erofs/inode.c | 18 --
> > 1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> > index 80f4fe9..6336cc1 100644
> > --- a/fs/erofs/inode.c
> > +++ b/fs/erofs/inode.c
> > @@ -190,22 +190,28 @@ static int fill_inode(struct inode *inode, int isdir)
> > err = read_inode(inode, data + ofs);
> > if (!err) {
> > /* setup the new inode */
> > - if (S_ISREG(inode->i_mode)) {
> > + switch (inode->i_mode & S_IFMT) {
> > + case S_IFREG:
> > inode->i_op = &erofs_generic_iops;
> > inode->i_fop = &generic_ro_fops;
> > - } else if (S_ISDIR(inode->i_mode)) {
> > + break;
> > + case S_IFDIR:
> > inode->i_op = &erofs_dir_iops;
> > inode->i_fop = &erofs_dir_fops;
> > - } else if (S_ISLNK(inode->i_mode)) {
> > + break;
> > + case S_IFLNK:
> > /* by default, page_get_link is used for symlink */
> > inode->i_op = &erofs_symlink_iops;
> > inode_nohighmem(inode);
> > - } else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) ||
> > - S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) {
> > + break;
> > + case S_IFCHR:
> > + case S_IFBLK:
> > + case S_IFIFO:
> > + case S_IFSOCK:
> > inode->i_op = &erofs_generic_iops;
> > init_special_inode(inode, inode->i_mode, inode->i_rdev);
> > goto out_unlock;
> > - } else {
> > + default:
> > err = -EFSCORRUPTED;
> > goto out_unlock;
> > }
> > --
> > 2.9.3
> >
> > ___
> > devel mailing list
> > de...@linuxdriverproject.org
> > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/7] erofs: on-disk format should have explicitly assigned numbers
On Fri, Aug 30, 2019 at 11:36:37AM +0800, Gao Xiang wrote:
> As Christoph claimed [1], on-disk format should have
> explicitly assigned numbers. I have to change it.
>
> [1] https://lore.kernel.org/r/20190829095954.gb20...@infradead.org/
> Reported-by: Christoph Hellwig
> Reviewed-by: Chao Yu
> Signed-off-by: Gao Xiang
...ignore this patchset. I will resend another incremental
patchset to address what Christoph suggested yesterday...
Thanks,
Gao Xiang
> ---
> no change
>
> fs/erofs/erofs_fs.h | 18 +-
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
> index afa7d45ca958..2447ad4d0920 100644
> --- a/fs/erofs/erofs_fs.h
> +++ b/fs/erofs/erofs_fs.h
> @@ -52,10 +52,10 @@ struct erofs_super_block {
> * 4~7 - reserved
> */
> enum {
> - EROFS_INODE_FLAT_PLAIN,
> - EROFS_INODE_FLAT_COMPRESSION_LEGACY,
> - EROFS_INODE_FLAT_INLINE,
> - EROFS_INODE_FLAT_COMPRESSION,
> + EROFS_INODE_FLAT_PLAIN = 0,
> + EROFS_INODE_FLAT_COMPRESSION_LEGACY = 1,
> + EROFS_INODE_FLAT_INLINE = 2,
> + EROFS_INODE_FLAT_COMPRESSION= 3,
> EROFS_INODE_LAYOUT_MAX
> };
>
> @@ -181,7 +181,7 @@ struct erofs_xattr_entry {
>
> /* available compression algorithm types */
> enum {
> - Z_EROFS_COMPRESSION_LZ4,
> + Z_EROFS_COMPRESSION_LZ4 = 0,
> Z_EROFS_COMPRESSION_MAX
> };
>
> @@ -239,10 +239,10 @@ struct z_erofs_map_header {
> *(di_advise could be 0, 1 or 2)
> */
> enum {
> - Z_EROFS_VLE_CLUSTER_TYPE_PLAIN,
> - Z_EROFS_VLE_CLUSTER_TYPE_HEAD,
> - Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD,
> - Z_EROFS_VLE_CLUSTER_TYPE_RESERVED,
> + Z_EROFS_VLE_CLUSTER_TYPE_PLAIN = 0,
> + Z_EROFS_VLE_CLUSTER_TYPE_HEAD = 1,
> + Z_EROFS_VLE_CLUSTER_TYPE_NONHEAD= 2,
> + Z_EROFS_VLE_CLUSTER_TYPE_RESERVED = 3,
> Z_EROFS_VLE_CLUSTER_TYPE_MAX
> };
>
> --
> 2.17.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: add exfat filesystem code to staging
On Thu, Aug 29, 2019 at 01:18:10PM +0200, Greg Kroah-Hartman wrote: > Hey, that's not nice, erofs isn't a POS. It could always use more > review, which the developers asked for numerous times. > > There's nothing different from a filesystem compared to a driver. If > its stand-alone, and touches nothing else, all issues with it are > self-contained and do not bother anyone else in the kernel. We merge > drivers all the time that need more work because our review cycles are > low. And review cycles for vfs developers are even more scarce than > driver reviewers. A lot of the issue that are trivial to pick are really just very basic issue that don't even require file system know how. Or in other ways just a little less lazy developer that looks out for similar code outside their own little fiefdom. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: add exfat filesystem code to staging
On Thu, Aug 29, 2019 at 10:56:31PM +0200, Pali Rohár wrote: > In my opinion, proper way should be to implement exFAT support into > existing fs/fat/ code instead of replacing whole vfat/msdosfs by this > new (now staging) fat implementation. > > In linux kernel we really do not need two different implementation of > VFAT32. Not only not useful, but having another one is actively harmful, as people might actually accidentally used it for classic fat. But what I'm really annoyed at is this whole culture of just dumping some crap into staging and hoping it'll sort itself out. Which it won't. We'll need a dedidcated developer spending some time on it and just get it into shape, and having it in staging does not help with that at all - it will get various random cleanup that could be trivially scripted, but that is rarely the main issue with any codebase. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: add exfat filesystem code to staging
On Friday 30 August 2019 08:40:06 Christoph Hellwig wrote: > On Thu, Aug 29, 2019 at 10:56:31PM +0200, Pali Rohár wrote: > > In my opinion, proper way should be to implement exFAT support into > > existing fs/fat/ code instead of replacing whole vfat/msdosfs by this > > new (now staging) fat implementation. > > > > In linux kernel we really do not need two different implementation of > > VFAT32. > > Not only not useful, but having another one is actively harmful, as > people might actually accidentally used it for classic fat. > > But what I'm really annoyed at is this whole culture of just dumping > some crap into staging and hoping it'll sort itself out. Which it > won't. We'll need a dedidcated developer spending some time on it > and just get it into shape, and having it in staging does not help > with that at all - it will get various random cleanup that could > be trivially scripted, but that is rarely the main issue with any > codebase. Exactly. Somebody should take this code and work on it. Otherwise we can say it is dead code and would happen same thing as with other staging drivers -- ready to be removed. -- Pali Rohár pali.ro...@gmail.com signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/7] erofs: some marcos are much more readable as a function
On Thu, Aug 29, 2019 at 08:16:27PM -0700, Joe Perches wrote:
> > - sizeof(__u32) * ((__count) - 1); })
> > +static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount)
> > +{
> > + unsigned int icount = le16_to_cpu(d_icount);
> > +
> > + if (!icount)
> > + return 0;
> > +
> > + return sizeof(struct erofs_xattr_ibody_header) +
> > + sizeof(__u32) * (icount - 1);
>
> Maybe use struct_size()?
Declaring a variable that is only used for struct_size is rather ugly.
But while we are nitpicking: you don't need to byteswap to check for 0,
so the local variable could be avoided.
Also what is that magic -1 for? Normally we use that for the
deprecated style where a variable size array is declared using
varname[1], but that doesn't seem to be the case for erofs.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
On Fri, Aug 30, 2019 at 11:36:42AM +0800, Gao Xiang wrote: > As Dan Carpenter suggested [1], I have to remove > all erofs likely/unlikely annotations. Do you have to remove all of them, or just those where you don't have a particularly good reason why you think in this particular case they might actually matter? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/7] erofs: some marcos are much more readable as a function
Hi Christoph,
On Fri, Aug 30, 2019 at 08:45:51AM -0700, Christoph Hellwig wrote:
> On Thu, Aug 29, 2019 at 08:16:27PM -0700, Joe Perches wrote:
> > > - sizeof(__u32) * ((__count) - 1); })
> > > +static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount)
> > > +{
> > > + unsigned int icount = le16_to_cpu(d_icount);
> > > +
> > > + if (!icount)
> > > + return 0;
> > > +
> > > + return sizeof(struct erofs_xattr_ibody_header) +
> > > + sizeof(__u32) * (icount - 1);
> >
> > Maybe use struct_size()?
>
> Declaring a variable that is only used for struct_size is rather ugly.
> But while we are nitpicking: you don't need to byteswap to check for 0,
> so the local variable could be avoided.
>
> Also what is that magic -1 for? Normally we use that for the
> deprecated style where a variable size array is declared using
> varname[1], but that doesn't seem to be the case for erofs.
I have to explain more about this (sorry about my awkward English)
here i_xattr_icount is to represent the size of xattr field of erofs, as
follows:
0 - no xattr at all (no erofs_xattr_ibody_header)
___
| inode |
|___|
1 - a erofs_xattr_ibody_header (12 byte) + 4-byte (shared + inline) xattrs
2 - a erofs_xattr_ibody_header (12 byte) + 8-byte (shared + inline) xattrs
(that is the magic -1 means...)
In order to keep the number continuously, actually the content could be
an array of shared_xattr_id and
an inline xattr combination (struct erofs_xattr_entry + name + value)
Thanks,
Gao Xiang
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/7] erofs: some marcos are much more readable as a function
On Fri, Aug 30, 2019 at 11:52:23PM +0800, Gao Xiang wrote:
> Hi Christoph,
>
> On Fri, Aug 30, 2019 at 08:45:51AM -0700, Christoph Hellwig wrote:
> > On Thu, Aug 29, 2019 at 08:16:27PM -0700, Joe Perches wrote:
> > > > - sizeof(__u32) * ((__count) - 1); })
> > > > +static inline unsigned int erofs_xattr_ibody_size(__le16 d_icount)
> > > > +{
> > > > + unsigned int icount = le16_to_cpu(d_icount);
> > > > +
> > > > + if (!icount)
> > > > + return 0;
> > > > +
> > > > + return sizeof(struct erofs_xattr_ibody_header) +
> > > > + sizeof(__u32) * (icount - 1);
> > >
> > > Maybe use struct_size()?
> >
> > Declaring a variable that is only used for struct_size is rather ugly.
> > But while we are nitpicking: you don't need to byteswap to check for 0,
> > so the local variable could be avoided.
> >
> > Also what is that magic -1 for? Normally we use that for the
> > deprecated style where a variable size array is declared using
> > varname[1], but that doesn't seem to be the case for erofs.
>
> I have to explain more about this (sorry about my awkward English)
> here i_xattr_icount is to represent the size of xattr field of erofs, as
> follows:
> 0 - no xattr at all (no erofs_xattr_ibody_header)
> ___
> | inode |
> |___|
>
> 1 - a erofs_xattr_ibody_header (12 byte) + 4-byte (shared + inline) xattrs
> 2 - a erofs_xattr_ibody_header (12 byte) + 8-byte (shared + inline) xattrs
>
> (that is the magic -1 means...)
>
> In order to keep the number continuously, actually the content could be
> an array of shared_xattr_id and
> an inline xattr combination (struct erofs_xattr_entry + name + value)
...Add a word, large xattrs should use shared xattr (which save xattrs
in another area) rather than inline xattr, shared xattr stores xattr_id
just after erofs_xattr_ibody_header and before inline xattrs...
Thanks,
Gao Xiang
>
> Thanks,
> Gao Xiang
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 6/7] erofs: remove all likely/unlikely annotations
Hi Christoph, On Fri, Aug 30, 2019 at 08:46:50AM -0700, Christoph Hellwig wrote: > On Fri, Aug 30, 2019 at 11:36:42AM +0800, Gao Xiang wrote: > > As Dan Carpenter suggested [1], I have to remove > > all erofs likely/unlikely annotations. > > Do you have to remove all of them, or just those where you don't have > a particularly good reason why you think in this particular case they > might actually matter? I just added unlikely/likely for all erofs error handling paths or rare happened cases at first... (That is all in my thought...) I don't have some benchmark data for each unlikely/likely case (and I have no idea "is that worth to take time to benchmark rather than do another more useful stuffs"), so..I have to kill them all... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page()
> - err = bio_add_page(bio, page, PAGE_SIZE, 0);
> - if (err != PAGE_SIZE) {
> + if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
> err = -EFAULT;
> goto err_out;
> }
This patch looks like an improvement. But looking at that whole
area just makes me cringe.
Why is there __erofs_get_meta_page with the two weird booleans instead
of a single erofs_get_meta_page that gets and gfp_t for additional
flags and an unsigned int for additional bio op flags.
Why do need ioprio support to start with? Seeing that in a new
fs look kinda odd. Do you have benchmarks that show the difference?
That function then calls erofs_grab_bio, which tries to handle a
bio_alloc failure, except that the function will not actually fail
due the mempool backing it. It also seems like and awfully
huge function to inline.
Why is there __submit_bio which really just obsfucates what is
going on? Also why is __submit_bio using bio_set_op_attrs instead
of opencode it as the comment right next to it asks you to?
Also I really don't understand why you can't just use read_cache_page
or even read_cache_page_gfp instead of __erofs_get_meta_page.
That function is a whole lot of duplication of functionality shared
by a lot of other file systems.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 20/24] erofs: introduce generic decompression backend
On Thu, Aug 15, 2019 at 12:41:51PM +0800, Gao Xiang wrote: > +static bool use_vmap; > +module_param(use_vmap, bool, 0444); > +MODULE_PARM_DESC(use_vmap, "Use vmap() instead of vm_map_ram() (default 0)"); And how would anyone know which to pick? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 03/24] erofs: add super block operations
On Thu, Aug 29, 2019 at 06:50:48PM +0800, Gao Xiang wrote:
> > Please use an erofs_ prefix for all your functions.
>
> It is already a static function, I have no idea what is wrong here.
Which part of all wasn't clear? Have you looked at the prefixes for
most functions in the various other big filesystems?
> > > + /* be careful RCU symlink path (see ext4_inode_info->i_data)! */
> > > + if (is_inode_fast_symlink(inode))
> > > + kfree(inode->i_link);
> >
> > is_inode_fast_symlink only shows up in a later patch. And really
> > obsfucates the check here in the only caller as you can just do an
> > unconditional kfree here - i_link will be NULL except for the case
> > where you explicitly set it.
>
> I cannot fully understand your point (sorry about my English),
> I will reply you about this later.
With that I mean that you should:
1) remove is_inode_fast_symlink and just opencode it in the few places
using it
2) remove the check in this place entirely as it is not needed
3) remove the comment quoted above as it is more confusing than not
having the comment
> > Is there any good reasons to use buffer heads like this in new code
> > vs directly using bios?
>
> This page can save in bdev page cache, it contains not only the erofs
> superblock so it can be fetched in page cache later.
If you want it in the page cache why not use read_mapping_page or similar?
> > > +/* set up default EROFS parameters */
> > > +static void default_options(struct erofs_sb_info *sbi)
> > > +{
> > > +}
> >
> > No need to add an empty function.
>
> Later patch will fill this function.
Please only add the function in the patch actually adding the
functionality.
> > > +}
> >
> > Why is this needed? You can just free your sb privatte information in
> > ->put_super and wire up kill_block_super as the ->kill_sb method
> > directly.
>
> See Al's comments,
> https://lore.kernel.org/r/20190720224955.gd17...@zeniv.linux.org.uk/
With that code it makes sense. In this paticular patch it does not.
So please add it only when actually needed.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 04/24] erofs: add raw address_space operations
On Thu, Aug 29, 2019 at 07:46:11PM +0800, Gao Xiang wrote: > Hi Christoph, > > On Thu, Aug 29, 2019 at 03:17:21AM -0700, Christoph Hellwig wrote: > > The actual address_space operations seem to largely duplicate > > the iomap versions. Please use those instead. Also I don't think > > any new file system should write up ->bmap these days. > > iomap doesn't support tail-end packing inline data till now, > I think Chao and I told you and Andreas before [1]. > > Since EROFS keeps a self-contained driver for now, we will use > iomap if it supports tail-end packing inline data later. Well, so work with the maintainers to enhance the core kernel. That is how Linux development works. We've added various iomap enhancements for gfs in the last merge windows, and we've added more for the brand new zonefs file system we plan to merge for 5.4. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 05/24] erofs: add inode operations
On Thu, Aug 29, 2019 at 07:59:22PM +0800, Gao Xiang wrote: > On Thu, Aug 29, 2019 at 03:24:26AM -0700, Christoph Hellwig wrote: > > [] > > > > > > + > > > + /* fill last page if inline data is available */ > > > + err = fill_inline_data(inode, data, ofs); > > > > Well, I think you should move the is_inode_flat_inline and > > (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) checks from that > > helper here, as otherwise you make everyone wonder why you'd always > > fill out the inline data. > > Currently, fill_inline_data() only fills for fast symlink, > later we can fill any tail-end block (such as dir block) > for our requirements. So change it when that later changes actually come in. And even then having the checks outside the function is a lot more obvious. > And I think that is minor. The problem is that each of these issues might appear minor on their own. But combined a lot of the coding style choices lead to code that is more suitable an obsfucated code contest than the Linux kernel as trying to understand even just a few places requires jumping through tons of helpers with misleading names and spread over various files. > The consideration is simply because iget_locked performs better > than iget5_locked. In what benchmark do the differences show up? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 7/7] erofs: redundant assignment in __erofs_get_meta_page()
Hi Christoph,
On Fri, Aug 30, 2019 at 09:28:12AM -0700, Christoph Hellwig wrote:
> > - err = bio_add_page(bio, page, PAGE_SIZE, 0);
> > - if (err != PAGE_SIZE) {
> > + if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
> > err = -EFAULT;
> > goto err_out;
> > }
>
> This patch looks like an improvement. But looking at that whole
> area just makes me cringe.
OK, I agree with you, I will improve it or just kill them all with
new iomap approach after it supports tail-end packing inline.
>
> Why is there __erofs_get_meta_page with the two weird booleans instead
> of a single erofs_get_meta_page that gets and gfp_t for additional
> flags and an unsigned int for additional bio op flags.
I agree with you. Thanks for your suggestion.
>
> Why do need ioprio support to start with? Seeing that in a new
> fs look kinda odd. Do you have benchmarks that show the difference?
I don't have some benchmark for all of these, can I just set
REQ_PRIO for all metadata? is that reasonable?
Could you kindly give some suggestion on this?
>
> That function then calls erofs_grab_bio, which tries to handle a
> bio_alloc failure, except that the function will not actually fail
> due the mempool backing it. It also seems like and awfully
> huge function to inline.
OK, I will simplify it. Thanks for your suggestion.
>
> Why is there __submit_bio which really just obsfucates what is
> going on? Also why is __submit_bio using bio_set_op_attrs instead
> of opencode it as the comment right next to it asks you to?
Originally, mainly due to backport consideration since some
of our smartphones use 3.x kernel as well...
>
> Also I really don't understand why you can't just use read_cache_page
> or even read_cache_page_gfp instead of __erofs_get_meta_page.
> That function is a whole lot of duplication of functionality shared
> by a lot of other file systems.
OK, I have to admit, that code was originally just copied from f2fs
with some modification (maybe it's not a good example for us).
Thanks,
Gao Xiang
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 20/24] erofs: introduce generic decompression backend
Hi Christoph, On Fri, Aug 30, 2019 at 09:35:34AM -0700, Christoph Hellwig wrote: > On Thu, Aug 15, 2019 at 12:41:51PM +0800, Gao Xiang wrote: > > +static bool use_vmap; > > +module_param(use_vmap, bool, 0444); > > +MODULE_PARM_DESC(use_vmap, "Use vmap() instead of vm_map_ram() (default > > 0)"); > > And how would anyone know which to pick? It has significant FIO benchmark difference on sequential read least on arm64... I have no idea whether all platform vm_map_ram() behaves better than vmap(), so I leave an option for users here... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 20/24] erofs: introduce generic decompression backend
On Sat, Aug 31, 2019 at 12:52:17AM +0800, Gao Xiang wrote: > Hi Christoph, > > On Fri, Aug 30, 2019 at 09:35:34AM -0700, Christoph Hellwig wrote: > > On Thu, Aug 15, 2019 at 12:41:51PM +0800, Gao Xiang wrote: > > > +static bool use_vmap; > > > +module_param(use_vmap, bool, 0444); > > > +MODULE_PARM_DESC(use_vmap, "Use vmap() instead of vm_map_ram() (default > > > 0)"); > > > > And how would anyone know which to pick? > > It has significant FIO benchmark difference on sequential read least on > arm64... > I have no idea whether all platform vm_map_ram() behaves better than vmap(), > so I leave an option for users here... vm_map_ram is supposed to generally behave better. So if it doesn't please report that that to the arch maintainer and linux-mm so that they can look into the issue. Having user make choices of deep down kernel internals is just a horrible interface. Please talk to maintainers of other bits of the kernel if you see issues and / or need enhancements. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 03/24] erofs: add super block operations
Hi Christoph,
On Fri, Aug 30, 2019 at 09:39:10AM -0700, Christoph Hellwig wrote:
> On Thu, Aug 29, 2019 at 06:50:48PM +0800, Gao Xiang wrote:
> > > Please use an erofs_ prefix for all your functions.
> >
> > It is already a static function, I have no idea what is wrong here.
>
> Which part of all wasn't clear? Have you looked at the prefixes for
> most functions in the various other big filesystems?
I will add erofs prefix to free_inode as you said.
At least, all non-prefix functions in erofs are all static functions,
it won't pollute namespace... I will add "erofs_" to other meaningful
callbacks...And as you can see...
cifs/cifsfs.c
1303:cifs_init_inodecache(void)
1509: rc = cifs_init_inodecache();
hpfs/super.c
254:static int init_inodecache(void)
771:int err = init_inodecache();
minix/inode.c
84:static int __init init_inodecache(void)
665:int err = init_inodecache();
isofs/inode.c
88:static int __init init_inodecache(void)
1580: int err = init_inodecache();
bfs/inode.c
261:static int __init init_inodecache(void)
468:int err = init_inodecache();
ext4/super.c
1144:static int __init init_inodecache(void)
6115: err = init_inodecache();
reiserfs/super.c
666:static int __init init_inodecache(void)
2606: ret = init_inodecache();
squashfs/super.c
406:static int __init init_inodecache(void)
430:int err = init_inodecache();
udf/super.c
177:static int __init init_inodecache(void)
232:err = init_inodecache();
qnx4/inode.c
358:static int init_inodecache(void)
399:err = init_inodecache();
ufs/super.c
1463:static int __init init_inodecache(void)
1517: int err = init_inodecache();
qnx6/inode.c
618:static int init_inodecache(void)
659:err = init_inodecache();
f2fs/super.c
3540:static int __init init_inodecache(void)
3572: err = init_inodecache();
>
> > > > + /* be careful RCU symlink path (see ext4_inode_info->i_data)! */
> > > > + if (is_inode_fast_symlink(inode))
> > > > + kfree(inode->i_link);
> > >
> > > is_inode_fast_symlink only shows up in a later patch. And really
> > > obsfucates the check here in the only caller as you can just do an
> > > unconditional kfree here - i_link will be NULL except for the case
> > > where you explicitly set it.
> >
> > I cannot fully understand your point (sorry about my English),
> > I will reply you about this later.
>
> With that I mean that you should:
>
> 1) remove is_inode_fast_symlink and just opencode it in the few places
> using it
> 2) remove the check in this place entirely as it is not needed
> 3) remove the comment quoted above as it is more confusing than not
> having the comment
Got it, thanks!
>
> > > Is there any good reasons to use buffer heads like this in new code
> > > vs directly using bios?
> >
> > This page can save in bdev page cache, it contains not only the erofs
> > superblock so it can be fetched in page cache later.
>
> If you want it in the page cache why not use read_mapping_page or similar?
It's reasonable, I will change as you suggested.
(The difference is whether it has some buffer_head to the sb page or not...)
>
> > > > +/* set up default EROFS parameters */
> > > > +static void default_options(struct erofs_sb_info *sbi)
> > > > +{
> > > > +}
> > >
> > > No need to add an empty function.
> >
> > Later patch will fill this function.
>
> Please only add the function in the patch actually adding the
> functionality.
That was my fault when spilting patches...considering
it's an >7KLOC filesystem (maybe spilting the whole xfs or
ext4 properly is more harder)... Anyway, that is my fault.
>
> > > > +}
> > >
> > > Why is this needed? You can just free your sb privatte information in
> > > ->put_super and wire up kill_block_super as the ->kill_sb method
> > > directly.
> >
> > See Al's comments,
> > https://lore.kernel.org/r/20190720224955.gd17...@zeniv.linux.org.uk/
>
> With that code it makes sense. In this paticular patch it does not.
> So please add it only when actually needed.
Same as above...
Thanks,
Gao Xiang
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 04/24] erofs: add raw address_space operations
Hi Christoph, On Fri, Aug 30, 2019 at 09:40:13AM -0700, Christoph Hellwig wrote: > On Thu, Aug 29, 2019 at 07:46:11PM +0800, Gao Xiang wrote: > > Hi Christoph, > > > > On Thu, Aug 29, 2019 at 03:17:21AM -0700, Christoph Hellwig wrote: > > > The actual address_space operations seem to largely duplicate > > > the iomap versions. Please use those instead. Also I don't think > > > any new file system should write up ->bmap these days. > > > > iomap doesn't support tail-end packing inline data till now, > > I think Chao and I told you and Andreas before [1]. > > > > Since EROFS keeps a self-contained driver for now, we will use > > iomap if it supports tail-end packing inline data later. > > Well, so work with the maintainers to enhance the core kernel. That > is how Linux development works. We've added various iomap enhancements > for gfs in the last merge windows, and we've added more for the brand > new zonefs file system we plan to merge for 5.4. That is a good idea, I think Chao will continue working on this (adding tail-end packing inline approach into iomap, thus we can have few code in data.c.) Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/8] media: cedrus: Detect first slice of a frame
Le vendredi 30 août 2019 à 07:48 +0200, Boris Brezillon a écrit :
> On Thu, 29 Aug 2019 21:04:28 +0200
> Jernej Škrabec wrote:
>
> > Dne ponedeljek, 26. avgust 2019 ob 20:28:31 CEST je Boris Brezillon
> > napisal(a):
> > > Hi Jernej,
> > >
> > > On Thu, 22 Aug 2019 21:44:57 +0200
> > >
> > > Jernej Skrabec wrote:
> > > > When codec supports multiple slices in one frame, VPU has to know when
> > > > first slice of each frame is being processed, presumably to correctly
> > > > clear/set data in auxiliary buffers.
> > > >
> > > > Add first_slice field to cedrus_run structure and set it according to
> > > > timestamps of capture and output buffers. If timestamps are different,
> > > > it's first slice and viceversa.
> > > >
> > > > Signed-off-by: Jernej Skrabec
> > > > ---
> > > >
> > > > drivers/staging/media/sunxi/cedrus/cedrus.h | 1 +
> > > > drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++
> > > > 2 files changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus.h index
> > > > 2f017a651848..32cb38e541c6 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > > @@ -70,6 +70,7 @@ struct cedrus_mpeg2_run {
> > > >
> > > > struct cedrus_run {
> > > >
> > > > struct vb2_v4l2_buffer *src;
> > > > struct vb2_v4l2_buffer *dst;
> > > >
> > > > + bool first_slice;
> > > >
> > > > union {
> > > >
> > > > struct cedrus_h264_run h264;
> > > >
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c index
> > > > 56ca4c9ad01c..d7b54accfe83 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > > > @@ -31,6 +31,8 @@ void cedrus_device_run(void *priv)
> > > >
> > > > run.src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > > > run.dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > > >
> > > > + run.first_slice =
> > > > + run.src->vb2_buf.timestamp != run.dst-
> > > vb2_buf.timestamp;
> > >
> > > Can't we use slice->first_mb_in_slice to determine if a slice is the
> > > first? I'd expect ->first_mb_in_slice to be 0 (unless we decide to
> > > support ASO).
> >
> > I looked in all VPU documentation available to me (which isn't much) and
> > there
> > is no indication if ASO is supported or not. Do you have any sample video
> > with
> > out-of-order slices? It's my understanding that this is uncommon.
>
> I'm not entirely sure, but my understanding was that it might be used
> when streaming over network where some packets might be lost and
> re-emitted later on.
>
> > If it's
> > supported, I would leave code as-is.
>
> I remember seeing the ASO acronym mentioned in the hantro G1 spec, but
> at the same time we're doing frame-based decoding, so I guess the HW
> block expects slices to be ordered in that case. Honestly I don't know,
> so let's keep the code as-is.
We had an ASO interrupt when we tried to do slice decoding rather then
frame. I believe on Hantro, the way to do ASO is to actually re-order
in software.
ASO is a feature of baseline profile use to reduce latency. As an
example, VA-API does not support baseline profile (only constrained-
baseline, which excludes ASO).
Nicolas
signature.asc
Description: This is a digitally signed message part
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: exfat: check for null return from call to FAT_getblk
From: Colin Ian King
A call to FAT_getblk is missing a null return check which can
lead to a null pointer dereference. Fix this by adding a null
check to match all the other FAT_getblk return sanity checks.
Addresses-Coverity: ("Dereference null return")
Fixes: c48c9f7ff32b ("staging: exfat: add exfat filesystem code to staging")
Signed-off-by: Colin Ian King
---
drivers/staging/exfat/exfat_cache.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/staging/exfat/exfat_cache.c
b/drivers/staging/exfat/exfat_cache.c
index f05d692c2b1e..1565ce65d39f 100644
--- a/drivers/staging/exfat/exfat_cache.c
+++ b/drivers/staging/exfat/exfat_cache.c
@@ -369,6 +369,8 @@ static s32 __FAT_write(struct super_block *sb, u32 loc, u32
content)
FAT_modify(sb, sec);
fat_sector = FAT_getblk(sb, ++sec);
+ if (!fat_sector)
+ return -1;
fat_sector[0] = (u8)((fat_sector[0] & 0xF0) |
(content >> 8));
} else {
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: exfat: remove redundant goto
From: Colin Ian King
The goto after a return is never executed, so it is redundant and can
be removed.
Addresses-Coverity: ("Structurally dead code")
Signed-off-by: Colin Ian King
---
drivers/staging/exfat/exfat_super.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/staging/exfat/exfat_super.c
b/drivers/staging/exfat/exfat_super.c
index 5b5c2ca8c9aa..5b3c4dfe0ecc 100644
--- a/drivers/staging/exfat/exfat_super.c
+++ b/drivers/staging/exfat/exfat_super.c
@@ -663,10 +663,8 @@ static int ffsLookupFile(struct inode *inode, char *path,
struct file_id_t *fid)
/* search the file name for directories */
dentry = p_fs->fs_func->find_dir_entry(sb, &dir, &uni_name, num_entries,
&dos_name, TYPE_ALL);
- if (dentry < -1) {
+ if (dentry < -1)
return FFS_NOTFOUND;
- goto out;
- }
fid->dir.dir = dir.dir;
fid->dir.size = dir.size;
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
staging: exfat: issue with FFS_MEDIAERR error return assignment
Hi,
Static analysis on exfat with Coverity has picked up an assignment of
FFS_MEDIAERR that gets over-written:
1750if (is_dir) {
1751if ((fid->dir.dir == p_fs->root_dir) &&
1752(fid->entry == -1)) {
1753if (p_fs->dev_ejected)
CID 85797 (#1 of 1): Unused value (UNUSED_VALUE)
Assigning value 1 to ret here, but that stored value is overwritten
before it can be used.
1754ret = FFS_MEDIAERR;
value_overwrite: Overwriting previous write to ret with value 0.
1755ret = FFS_SUCCESS;
1756goto out;
1757}
1758}
I doubt that's intentional, should it be instead the following?
if (p_fs->dev_ejected)
ret = FFS_MEDIAERR;
else
ret = FFS_SUCCESS;
goto out;
Colin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 05/24] erofs: add inode operations
Hi Christoph, On Fri, Aug 30, 2019 at 09:42:05AM -0700, Christoph Hellwig wrote: > On Thu, Aug 29, 2019 at 07:59:22PM +0800, Gao Xiang wrote: > > On Thu, Aug 29, 2019 at 03:24:26AM -0700, Christoph Hellwig wrote: > > > > [] > > > > > > > > > + > > > > + /* fill last page if inline data is available */ > > > > + err = fill_inline_data(inode, data, ofs); > > > > > > Well, I think you should move the is_inode_flat_inline and > > > (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) checks from that > > > helper here, as otherwise you make everyone wonder why you'd always > > > fill out the inline data. > > > > Currently, fill_inline_data() only fills for fast symlink, > > later we can fill any tail-end block (such as dir block) > > for our requirements. > > So change it when that later changes actually come in. And even then > having the checks outside the function is a lot more obvious. Okay. > > > And I think that is minor. > > The problem is that each of these issues might appear minor on their > own. But combined a lot of the coding style choices lead to code that > is more suitable an obsfucated code contest than the Linux kernel as > trying to understand even just a few places requires jumping through > tons of helpers with misleading names and spread over various files. > > > The consideration is simply because iget_locked performs better > > than iget5_locked. > > In what benchmark do the differences show up? In a word, no benchmark here, just because "unsigned long on 32-bit platforms is 4 bytes." but erofs nid is a 64-bit number. iget_locked will do find_inode_fast (no callback at all) rather than iget5_locked --> find_inode (test callback) -> inode_insert5(set callback) for each new inode. For most 64-bit platforms, iget_locked is enough, 32-bit platforms become rare... Thanks, Gao Xiang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: exfat: fix uninitialized variable ret
From: Colin Ian King
Currently there are error return paths in ffsReadFile that
exit via lable err_out that return and uninitialized error
return in variable ret. Fix this by initializing ret to zero.
Addresses-Coverity: ("Uninitialized scalar variable")
Fixes: c48c9f7ff32b ("staging: exfat: add exfat filesystem code to staging")
Signed-off-by: Colin Ian King
---
drivers/staging/exfat/exfat_super.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/exfat/exfat_super.c
b/drivers/staging/exfat/exfat_super.c
index 5b3c4dfe0ecc..6939aa4f25ee 100644
--- a/drivers/staging/exfat/exfat_super.c
+++ b/drivers/staging/exfat/exfat_super.c
@@ -771,7 +771,7 @@ static int ffsReadFile(struct inode *inode, struct
file_id_t *fid, void *buffer,
{
s32 offset, sec_offset, clu_offset;
u32 clu;
- int ret;
+ int ret = 0;
sector_t LogSector;
u64 oneblkread, read_bytes;
struct buffer_head *tmp_bh = NULL;
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/4] binder: Add stats, state and transactions files
On Fri, Aug 30, 2019 at 4:32 AM Christian Brauner
wrote:
>
> On Thu, Aug 29, 2019 at 02:18:10PM -0700, Hridya Valsaraju wrote:
> > The following binder stat files currently live in debugfs.
> >
> > /sys/kernel/debug/binder/state
> > /sys/kernel/debug/binder/stats
> > /sys/kernel/debug/binder/transactions
> >
> > This patch makes these files available in a binderfs instance
> > mounted with the mount option 'stats=global'. For example, if a binderfs
> > instance is mounted at path /dev/binderfs, the above files will be
> > available at the following locations:
> >
> > /dev/binderfs/binder_logs/state
> > /dev/binderfs/binder_logs/stats
> > /dev/binderfs/binder_logs/transactions
> >
> > This provides a way to access them even when debugfs is not mounted.
> >
> > Signed-off-by: Hridya Valsaraju
>
> Just two comments below. If you have addressed them you can add my:
>
> Acked-by: Christian Brauner
Thank you for taking another look Christian, will address both
comments and send out v3 soon :)
>
> > ---
> >
> > Changes in v2:
> > - Consistently name variables across functions as per Christian
> >Brauner.
> > - Improve check for binderfs device in binderfs_evict_inode()
> >as per Christian Brauner.
> >
> > drivers/android/binder.c | 15 ++--
> > drivers/android/binder_internal.h | 8 ++
> > drivers/android/binderfs.c| 140 +-
> > 3 files changed, 153 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index ca6b21a53321..de795bd229c4 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -6055,7 +6055,7 @@ static void print_binder_proc_stats(struct seq_file
> > *m,
> > }
> >
> >
> > -static int state_show(struct seq_file *m, void *unused)
> > +int binder_state_show(struct seq_file *m, void *unused)
> > {
> > struct binder_proc *proc;
> > struct binder_node *node;
> > @@ -6094,7 +6094,7 @@ static int state_show(struct seq_file *m, void
> > *unused)
> > return 0;
> > }
> >
> > -static int stats_show(struct seq_file *m, void *unused)
> > +int binder_stats_show(struct seq_file *m, void *unused)
> > {
> > struct binder_proc *proc;
> >
> > @@ -6110,7 +6110,7 @@ static int stats_show(struct seq_file *m, void
> > *unused)
> > return 0;
> > }
> >
> > -static int transactions_show(struct seq_file *m, void *unused)
> > +int binder_transactions_show(struct seq_file *m, void *unused)
> > {
> > struct binder_proc *proc;
> >
> > @@ -6198,9 +6198,6 @@ const struct file_operations binder_fops = {
> > .release = binder_release,
> > };
> >
> > -DEFINE_SHOW_ATTRIBUTE(state);
> > -DEFINE_SHOW_ATTRIBUTE(stats);
> > -DEFINE_SHOW_ATTRIBUTE(transactions);
> > DEFINE_SHOW_ATTRIBUTE(transaction_log);
> >
> > static int __init init_binder_device(const char *name)
> > @@ -6256,17 +6253,17 @@ static int __init binder_init(void)
> > 0444,
> > binder_debugfs_dir_entry_root,
> > NULL,
> > - &state_fops);
> > + &binder_state_fops);
> > debugfs_create_file("stats",
> > 0444,
> > binder_debugfs_dir_entry_root,
> > NULL,
> > - &stats_fops);
> > + &binder_stats_fops);
> > debugfs_create_file("transactions",
> > 0444,
> > binder_debugfs_dir_entry_root,
> > NULL,
> > - &transactions_fops);
> > + &binder_transactions_fops);
> > debugfs_create_file("transaction_log",
> > 0444,
> > binder_debugfs_dir_entry_root,
> > diff --git a/drivers/android/binder_internal.h
> > b/drivers/android/binder_internal.h
> > index fe8c745dc8e0..12ef96f256c6 100644
> > --- a/drivers/android/binder_internal.h
> > +++ b/drivers/android/binder_internal.h
> > @@ -57,4 +57,12 @@ static inline int __init init_binderfs(void)
> > }
> > #endif
> >
> > +int binder_stats_show(struct seq_file *m, void *unused);
> > +DEFINE_SHOW_ATTRIBUTE(binder_stats);
> > +
> > +int binder_state_show(struct seq_file *m, void *unused);
> > +DEFINE_SHOW_ATTRIBUTE(binder_state);
> > +
> > +int binder_transactions_show(struct seq_file *m, void *unused);
> > +DEFINE_SHOW_ATTRIBUTE(binder_transactions);
> > #endif /* _LINUX_BINDER_INTERNAL_H */
> > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> > index 7045bfe5b52b..0e1e7c87cd33 100644
> > --- a/drivers/android/binderfs.c
> > +++ b/drivers/android/binderfs.c
> > @@ -280,7 +280,7 @@ static void binderfs_evict_inode(struct inode *inode)
>
Re: [PATCH] staging: exfat: add exfat filesystem code to staging
On Thu, Aug 29, 2019 at 01:18:10PM +0200, Greg Kroah-Hartman wrote: > On Thu, Aug 29, 2019 at 03:37:49AM -0700, Christoph Hellwig wrote: > > On Thu, Aug 29, 2019 at 11:50:19AM +0200, Greg Kroah-Hartman wrote: > > > I know the code is horrible, but I will gladly take horrible code into > > > staging. If it bothers you, just please ignore it. That's what staging > > > is there for :) > > > > And then after a while you decide it's been long enough and force move > > it out of staging like the POS erofs code? > > Hey, that's not nice, erofs isn't a POS. It could always use more > review, which the developers asked for numerous times. > > There's nothing different from a filesystem compared to a driver. If > its stand-alone, and touches nothing else, all issues with it are > self-contained and do not bother anyone else in the kernel. We merge I whole-heartedly disagree with that statement. The major difference between filesystems and the rest of the kernel that seems to be missed by most kernel developers is that filesystems maintain persistent data - you can't fix a problem/bug by rebooting or power cycling. Once the filesystem code screws up, the user is left with a mess they have to fix and that invariably results in data loss. Users remember when a filesystem eats their data - they don't tend to want to have anything to do with that filesystem ever again if it happens to them. We still get people saying "XFS ate my data back in 2002, I dont trust it and I'll never use it again". Users put up with shit hardware and drivers - it's an inconvenience more than anything. They don't put up with buggy filesystems that lose their data - that is completely unacceptible to users. As a result, the quality and stability standard for merging a new filesystem needs to be far higher that what is acceptible for merging a new driver. The correct place for new filesystem review is where all the experienced filesystem developers hang out - that's linux-fsdevel, not the driver staging tree. Cheers, Dave. -- Dave Chinner da...@fromorbit.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: rts5208: Fix checkpath warning
Hi SAI,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc6 next-20190830]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/P-SAI-PRASANTH/staging-rts5208-Fix-checkpath-warning/20190831-034841
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.4.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=ia64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All errors (new ones prefixed by >>):
drivers/staging//rts5208/rtsx_transport.c: In function
'rtsx_transfer_sglist_adma':
>> drivers/staging//rts5208/rtsx_transport.c:548:4: error: expected ';' before
>> 'rtsx_add_sg_tbl'
rtsx_add_sg_tbl(chip, (u32)addr, (u32)len, option);
^~~
vim +548 drivers/staging//rts5208/rtsx_transport.c
fa590c222fbaa4 Micky Ching 2013-11-12 479
fa590c222fbaa4 Micky Ching 2013-11-12 480 static int
rtsx_transfer_sglist_adma(struct rtsx_chip *chip, u8 card,
fa590c222fbaa4 Micky Ching 2013-11-12 481
struct scatterlist *sg, int num_sg,
d1303c1a9a68e5 Shaun Ren 2016-02-15 482
enum dma_data_direction dma_dir,
d1303c1a9a68e5 Shaun Ren 2016-02-15 483
int timeout)
fa590c222fbaa4 Micky Ching 2013-11-12 484 {
fa590c222fbaa4 Micky Ching 2013-11-12 485struct rtsx_dev *rtsx =
chip->rtsx;
fa590c222fbaa4 Micky Ching 2013-11-12 486struct completion
trans_done;
fa590c222fbaa4 Micky Ching 2013-11-12 487u8 dir;
fa590c222fbaa4 Micky Ching 2013-11-12 488int buf_cnt, i;
fa590c222fbaa4 Micky Ching 2013-11-12 489int err = 0;
fa590c222fbaa4 Micky Ching 2013-11-12 490long timeleft;
fa590c222fbaa4 Micky Ching 2013-11-12 491struct scatterlist
*sg_ptr;
fa590c222fbaa4 Micky Ching 2013-11-12 492
50dcad2a2c85d5 Shaun Ren 2016-02-15 493if (!sg || (num_sg <=
0))
fa590c222fbaa4 Micky Ching 2013-11-12 494return -EIO;
fa590c222fbaa4 Micky Ching 2013-11-12 495
fa590c222fbaa4 Micky Ching 2013-11-12 496if (dma_dir ==
DMA_TO_DEVICE)
fa590c222fbaa4 Micky Ching 2013-11-12 497dir =
HOST_TO_DEVICE;
fa590c222fbaa4 Micky Ching 2013-11-12 498else if (dma_dir ==
DMA_FROM_DEVICE)
fa590c222fbaa4 Micky Ching 2013-11-12 499dir =
DEVICE_TO_HOST;
fa590c222fbaa4 Micky Ching 2013-11-12 500else
fa590c222fbaa4 Micky Ching 2013-11-12 501return -ENXIO;
fa590c222fbaa4 Micky Ching 2013-11-12 502
fa590c222fbaa4 Micky Ching 2013-11-12 503if (card == SD_CARD)
fa590c222fbaa4 Micky Ching 2013-11-12 504
rtsx->check_card_cd = SD_EXIST;
fa590c222fbaa4 Micky Ching 2013-11-12 505else if (card ==
MS_CARD)
fa590c222fbaa4 Micky Ching 2013-11-12 506
rtsx->check_card_cd = MS_EXIST;
fa590c222fbaa4 Micky Ching 2013-11-12 507else if (card ==
XD_CARD)
fa590c222fbaa4 Micky Ching 2013-11-12 508
rtsx->check_card_cd = XD_EXIST;
fa590c222fbaa4 Micky Ching 2013-11-12 509else
fa590c222fbaa4 Micky Ching 2013-11-12 510
rtsx->check_card_cd = 0;
fa590c222fbaa4 Micky Ching 2013-11-12 511
fa590c222fbaa4 Micky Ching 2013-11-12 512
spin_lock_irq(&rtsx->reg_lock);
fa590c222fbaa4 Micky Ching 2013-11-12 513
fa590c222fbaa4 Micky Ching 2013-11-12 514/* set up data
structures for the wakeup system */
fa590c222fbaa4 Micky Ching 2013-11-12 515rtsx->done =
&trans_done;
fa590c222fbaa4 Micky Ching 2013-11-12 516
fa590c222fbaa4 Micky Ching 2013-11-12 517rtsx->trans_state =
STATE_TRANS_SG;
fa590c222fbaa4 Micky Ching 2013-11-12 518rtsx->trans_result =
TRANS_NOT_READY;
fa590c222fbaa4 Micky Ching 2013-11-12 519
fa590c222fbaa4 Micky Ching 2013-11-12 520
spin_unlock_irq(&rtsx->reg_lock);
fa590c222fbaa4 Micky Ching 2013-11-12 521
69e3bc543c5610 Shaun Ren 2016-02-15 522buf_cnt =
dma_map_sg(&rtsx->pci->dev, sg, num_sg, dma_dir);
fa590c222fbaa4 Micky Ching 2013-11-12 523
fa590c222fbaa4 Micky Ching 2013-11-12 524sg_ptr = sg;
fa590c222fbaa4 Micky Ching 2013-11-12 525
fa590c222fbaa4 Micky Ching 2013-11-
Singaporean Mr. Teo En Ming's Refugee Seeking Attempts, In The Search of a Substantially Better Life
Subject: Singaporean Mr. Teo En Ming's Refugee Seeking Attempts, In The Search of a Substantially Better Life In reverse chronological order: [1] Petition to the Government of Taiwan for Refugee Status, 5th August 2019 Monday Photo #1: At the building of the National Immigration Agency, Ministry of the Interior, Taipei, Taiwan, 5th August 2019 Photo #2: Queue ticket no. 515 at the National Immigration Agency, Ministry of the Interior, Taipei, Taiwan, 5th August 2019 Photo #3: Submission of documents/petition to the National Immigration Agency, Ministry of the Interior, Taipei, Taiwan, 5th August 2019 Photos #4 and #5: Acknowledgement of Receipt (no. 03142) for the submission of documents/petition from the National Immigration Agency, Ministry of the Interior, Taipei, Taiwan, 5th August 2019, 10:00 AM References: (a) Petition to the Government of Taiwan for Refugee Status, 5th August 2019 Monday (Blogspot blog) Link: https://tdtemcerts.blogspot.sg/2019/08/petition-to-government-of-taiwan-for.html (b) Petition to the Government of Taiwan for Refugee Status, 5th August 2019 Monday (Wordpress blog) Link: https://tdtemcerts.wordpress.com/2019/08/23/petition-to-the-government-of-taiwan-for-refugee-status/ [2] Application for Refugee Status at the United Nations Refugee Agency, Bangkok, Thailand, 21st March 2017 Tuesday References: (a) [YOUTUBE] Vlog: The Road to Application for Refugee Status at the United Nations High Commissioner for Refugees, Bangkok Link: https://www.youtube.com/watch?v=utpuAa1eUNI YouTube video Published on March 22nd, 2017 Views as at 31st August 2019: 593 YouTube Channel: Turritopsis Dohrnii Teo En Ming Subscribers as at 31st August 2019: 2815 Link: https://www.youtube.com/channel/UC__F2hzlqNEEGx-IXxQi3hA -BEGIN EMAIL SIGNATURE- The Gospel for all Targeted Individuals (TIs): [The New York Times] Microwave Weapons Are Prime Suspect in Ills of U.S. Embassy Workers Link: https://www.nytimes.com/2018/09/01/science/sonic-attack-cuba-microwave.html Singaporean Mr. Turritopsis Dohrnii Teo En Ming's Academic Qualifications as at 14 Feb 2019 [1] https://tdtemcerts.wordpress.com/ [2] https://tdtemcerts.blogspot.sg/ [3] https://www.scribd.com/user/270125049/Teo-En-Ming -END EMAIL SIGNATURE- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 03/24] erofs: add super block operations
Hi Christoph,
On Sat, Aug 31, 2019 at 01:15:10AM +0800, Gao Xiang wrote:
[]
> >
> > > > > + /* be careful RCU symlink path (see ext4_inode_info->i_data)! */
> > > > > + if (is_inode_fast_symlink(inode))
> > > > > + kfree(inode->i_link);
> > > >
> > > > is_inode_fast_symlink only shows up in a later patch. And really
> > > > obsfucates the check here in the only caller as you can just do an
> > > > unconditional kfree here - i_link will be NULL except for the case
> > > > where you explicitly set it.
> > >
> > > I cannot fully understand your point (sorry about my English),
> > > I will reply you about this later.
> >
> > With that I mean that you should:
> >
> > 1) remove is_inode_fast_symlink and just opencode it in the few places
> > using it
> > 2) remove the check in this place entirely as it is not needed
Add some words about this suggestion since I'm addressing this place, it
seems it could not (or I am not sure at least) be freed unconditionally
union {
struct pipe_inode_info *i_pipe;
struct block_device *i_bdev;
struct cdev *i_cdev;
char*i_link;
unsignedi_dir_seq;
};
while I saw what shmem did, it seems that they handle as follows:
3636 static void shmem_free_in_core_inode(struct inode *inode)
3637 {
3638 if (S_ISLNK(inode->i_mode))
3639 kfree(inode->i_link);
3640 kmem_cache_free(shmem_inode_cachep, SHMEM_I(inode));
3641 }
I think that would be some check on it to get it is a symlink (for
i_dir_seq it seems unsafe) I think the original check is ok but
I will opencode it instead.
Thanks,
Gao Xiang
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3] staging: rts5208: Fix checkpath warning
This patch fixes the following checkpath warning in the file drivers/staging/rts5208/rtsx_transport.c:546 WARNING: line over 80 characters + option = RTSX_SG_VALID | RTSX_SG_END | RTSX_SG_TRANS_DATA; Signed-off-by: P SAI PRASANTH --- Changes in v3: -Fixes the following error in da59abd45efc >> drivers/staging//rts5208/rtsx_transport.c:548:4: error: expected ';' before 'rtsx_add_sg_tbl' rtsx_add_sg_tbl(chip, (u32)addr, (u32)len, option); ^~~ drivers/staging/rts5208/rtsx_transport.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/staging/rts5208/rtsx_transport.c b/drivers/staging/rts5208/rtsx_transport.c index 8277d78..3c67656 100644 --- a/drivers/staging/rts5208/rtsx_transport.c +++ b/drivers/staging/rts5208/rtsx_transport.c @@ -540,11 +540,10 @@ static int rtsx_transfer_sglist_adma(struct rtsx_chip *chip, u8 card, dev_dbg(rtsx_dev(chip), "DMA addr: 0x%x, Len: 0x%x\n", (unsigned int)addr, len); - + + option = RTSX_SG_VALID | RTSX_SG_TRANS_DATA; if (j == (sg_cnt - 1)) - option = RTSX_SG_VALID | RTSX_SG_END | RTSX_SG_TRANS_DATA; - else - option = RTSX_SG_VALID | RTSX_SG_TRANS_DATA; + option |= RTSX_SG_END; rtsx_add_sg_tbl(chip, (u32)addr, (u32)len, option); -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] staging: rts5208: Fix checkpath warning
On Sat, 2019-08-31 at 07:55 +0530, P SAI PRASANTH wrote: > This patch fixes the following checkpath warning > in the file drivers/staging/rts5208/rtsx_transport.c:546 > > WARNING: line over 80 characters > + option = RTSX_SG_VALID | RTSX_SG_END | > RTSX_SG_TRANS_DATA; [] > diff --git a/drivers/staging/rts5208/rtsx_transport.c > b/drivers/staging/rts5208/rtsx_transport.c [] > @@ -540,11 +540,10 @@ static int rtsx_transfer_sglist_adma(struct rtsx_chip > *chip, u8 card, > > dev_dbg(rtsx_dev(chip), "DMA addr: 0x%x, Len: 0x%x\n", > (unsigned int)addr, len); > - > + This same line delete then insert the same blank line is very unusual. What did you use to create this patch? > + option = RTSX_SG_VALID | RTSX_SG_TRANS_DATA; > if (j == (sg_cnt - 1)) > - option = RTSX_SG_VALID | RTSX_SG_END | > RTSX_SG_TRANS_DATA; > - else > - option = RTSX_SG_VALID | RTSX_SG_TRANS_DATA; > + option |= RTSX_SG_END; > > rtsx_add_sg_tbl(chip, (u32)addr, (u32)len, option); > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] staging: rts5208: Fix checkpath warning
On 19-08-30 19:58:09, Joe Perches wrote: > On Sat, 2019-08-31 at 07:55 +0530, P SAI PRASANTH wrote: > > This patch fixes the following checkpath warning > > in the file drivers/staging/rts5208/rtsx_transport.c:546 > > > > WARNING: line over 80 characters > > + option = RTSX_SG_VALID | RTSX_SG_END | > > RTSX_SG_TRANS_DATA; > [] > > diff --git a/drivers/staging/rts5208/rtsx_transport.c > > b/drivers/staging/rts5208/rtsx_transport.c > [] > > @@ -540,11 +540,10 @@ static int rtsx_transfer_sglist_adma(struct rtsx_chip > > *chip, u8 card, > > > > dev_dbg(rtsx_dev(chip), "DMA addr: 0x%x, Len: 0x%x\n", > > (unsigned int)addr, len); > > - > > + > > This same line delete then insert the same blank line > is very unusual. > > What did you use to create this patch? > > > + option = RTSX_SG_VALID | RTSX_SG_TRANS_DATA; > > if (j == (sg_cnt - 1)) > > - option = RTSX_SG_VALID | RTSX_SG_END | > > RTSX_SG_TRANS_DATA; > > - else > > - option = RTSX_SG_VALID | RTSX_SG_TRANS_DATA; > > + option |= RTSX_SG_END; > > > > rtsx_add_sg_tbl(chip, (u32)addr, (u32)len, option); > > > I have used vim for creating this patch. Upon checking, there is an extra tab on the new blankline which has been added. I'm sending an update immediately. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4] staging: rts5208: Fix checkpath warning
This patch fixes the following checkpath warning in the file drivers/staging/rts5208/rtsx_transport.c:546 WARNING: line over 80 characters + option = RTSX_SG_VALID | RTSX_SG_END | RTSX_SG_TRANS_DATA; Signed-off-by: P SAI PRASANTH --- Changes in v4: -Fix extra tab issue which was causing same blank line to be added and removed in git diff. drivers/staging/rts5208/rtsx_transport.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/staging/rts5208/rtsx_transport.c b/drivers/staging/rts5208/rtsx_transport.c index 8277d78..c1d99c4 100644 --- a/drivers/staging/rts5208/rtsx_transport.c +++ b/drivers/staging/rts5208/rtsx_transport.c @@ -541,10 +541,9 @@ static int rtsx_transfer_sglist_adma(struct rtsx_chip *chip, u8 card, dev_dbg(rtsx_dev(chip), "DMA addr: 0x%x, Len: 0x%x\n", (unsigned int)addr, len); + option = RTSX_SG_VALID | RTSX_SG_TRANS_DATA; if (j == (sg_cnt - 1)) - option = RTSX_SG_VALID | RTSX_SG_END | RTSX_SG_TRANS_DATA; - else - option = RTSX_SG_VALID | RTSX_SG_TRANS_DATA; + option |= RTSX_SG_END; rtsx_add_sg_tbl(chip, (u32)addr, (u32)len, option); -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: exfat: add exfat filesystem code to staging
On 2019/8/30 19:51, David Sterba wrote: > On Fri, Aug 30, 2019 at 10:06:25AM +0800, Chao Yu wrote: >> On 2019/8/29 23:43, Dan Carpenter wrote: p.s. There are 2947 (un)likely places in fs/ directory. >>> >>> I was complaining about you adding new pointless ones, not existing >>> ones. The likely/unlikely annotations are supposed to be functional and >>> not decorative. I explained this very clearly. >>> >>> Probably most of the annotations in fs/ are wrong but they are also >>> harmless except for the slight messiness. However there are definitely >>> some which are important so removing them all isn't a good idea. >> >> Hi Dan, >> >> Could you please pick up one positive example using likely and unlikely >> correctly? so we can follow the example, rather than removing them all >> blindly. > > Remove all of them and re-add with explanation if and how each is going > to make things better. If you can't reason about, prove by benchmarks or > point to inefficient asm code generated, then don't add them again. It seems the result of it is strongly related to arch and compiler, if we readd it after we just prove its gain only in one combination, I doubt we may suffer regression in other combination in between archs and comilers. It looks like we don't have any cheaper way to readd it? instead of verifying all/most combinations. > > The feedback I got from CPU and compiler people over the years is not to > bother using the annotations. CPUs are doing dynamic branch prediction > and compilers are applying tons of optimizations. > > GCC docs state about the builtin: "In general, you should prefer to use > actual profile feedback for this (-fprofile-arcs), as programmers are > notoriously bad at predicting how their programs actually perform." > (https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html) Yes, I agree with this. Thanks a lot for sharing your experience. :) The removal change has done and been merged into Greg's tree, let's consider to readd it later if possible as you suggested. thanks, > . > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 03/24] erofs: add super block operations
On Fri, Aug 30, 2019 at 8:16 PM Gao Xiang wrote: > > Hi Christoph, > > On Fri, Aug 30, 2019 at 09:39:10AM -0700, Christoph Hellwig wrote: > > On Thu, Aug 29, 2019 at 06:50:48PM +0800, Gao Xiang wrote: > > > > Please use an erofs_ prefix for all your functions. > > > > > > It is already a static function, I have no idea what is wrong here. > > > > Which part of all wasn't clear? Have you looked at the prefixes for > > most functions in the various other big filesystems? > > I will add erofs prefix to free_inode as you said. > > At least, all non-prefix functions in erofs are all static functions, > it won't pollute namespace... I will add "erofs_" to other meaningful > callbacks...And as you can see... > > cifs/cifsfs.c > 1303:cifs_init_inodecache(void) > 1509: rc = cifs_init_inodecache(); > > hpfs/super.c > 254:static int init_inodecache(void) > 771:int err = init_inodecache(); > > minix/inode.c > 84:static int __init init_inodecache(void) > 665:int err = init_inodecache(); > Hi Gao, "They did it first" is never a good reply for code review comments. Nobody cares if you copy&paste code with init_inodecache(). I understand why you thought static function names do not pollute the (linker) namespace, but they do pollute the global namespace. free_inode() as a local function name is one of the worst examples for VFS namespace pollution. VFS code uses function names like those a lot in the global namespace, e.g.: clear_inode(),new_inode(). For example from recent history of namespace collision caused by your line of thinking, see: e6fd2093a85d md: namespace private helper names Besides, you really have nothing to loose from prefixing everything with erofs_, do you? It's better for review, for debugging... Thanks, Amir. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 03/24] erofs: add super block operations
On Sat, Aug 31, 2019 at 09:34:44AM +0300, Amir Goldstein wrote: > On Fri, Aug 30, 2019 at 8:16 PM Gao Xiang wrote: > > > > Hi Christoph, > > > > On Fri, Aug 30, 2019 at 09:39:10AM -0700, Christoph Hellwig wrote: > > > On Thu, Aug 29, 2019 at 06:50:48PM +0800, Gao Xiang wrote: > > > > > Please use an erofs_ prefix for all your functions. > > > > > > > > It is already a static function, I have no idea what is wrong here. > > > > > > Which part of all wasn't clear? Have you looked at the prefixes for > > > most functions in the various other big filesystems? > > > > I will add erofs prefix to free_inode as you said. > > > > At least, all non-prefix functions in erofs are all static functions, > > it won't pollute namespace... I will add "erofs_" to other meaningful > > callbacks...And as you can see... > > > > cifs/cifsfs.c > > 1303:cifs_init_inodecache(void) > > 1509: rc = cifs_init_inodecache(); > > > > hpfs/super.c > > 254:static int init_inodecache(void) > > 771:int err = init_inodecache(); > > > > minix/inode.c > > 84:static int __init init_inodecache(void) > > 665:int err = init_inodecache(); > > > > Hi Gao, > > "They did it first" is never a good reply for code review comments. > Nobody cares if you copy&paste code with init_inodecache(). > I understand why you thought static function names do not pollute > the (linker) namespace, but they do pollute the global namespace. > > free_inode() as a local function name is one of the worst examples > for VFS namespace pollution. > > VFS code uses function names like those a lot in the global namespace, e.g.: > clear_inode(),new_inode(). > > For example from recent history of namespace collision caused by your line > of thinking, see: > e6fd2093a85d md: namespace private helper names > > Besides, you really have nothing to loose from prefixing everything > with erofs_, do you? It's better for review, for debugging... Hi Amir, Thanks for you kind reply... Yes, I understand that some generic header files could have the same function names and cause bad behaviors... I will fix them, my only one question is "if all function/variable names are prefixed with "erofs_" (including all inline helpers in header files), it seems somewhat strange... (too many statements start "erofs_" in the source code...)" I will fix common and short names at once... Thanks, Gao Xiang > > Thanks, > Amir. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
