| Message ID | 20220512130321.30599-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | imon_raw: respect DMA coherency | expand |
Hi Oliver,
I love your patch! Perhaps something to improve:
[auto build test WARNING on media-tree/master]
[also build test WARNING on v5.18-rc6 next-20220512]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Oliver-Neukum/imon_raw-respect-DMA-coherency/20220512-210422
base: git://linuxtv.org/media_tree.git master
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220513/202205130146.FkLP3KTa-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/5e1a1b1e9c8288033f5f1f1d70a3d7506114fad3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Oliver-Neukum/imon_raw-respect-DMA-coherency/20220512-210422
git checkout 5e1a1b1e9c8288033f5f1f1d70a3d7506114fad3
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/media/rc/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/byteorder/big_endian.h:5,
from arch/arc/include/uapi/asm/byteorder.h:14,
from include/asm-generic/bitops/le.h:6,
from arch/arc/include/asm/bitops.h:192,
from include/linux/bitops.h:33,
from include/linux/log2.h:12,
from include/asm-generic/div64.h:55,
from ./arch/arc/include/generated/asm/div64.h:1,
from include/linux/math.h:6,
from include/linux/math64.h:6,
from include/linux/time.h:6,
from include/linux/stat.h:19,
from include/linux/module.h:13,
from drivers/media/rc/imon_raw.c:5:
drivers/media/rc/imon_raw.c: In function 'imon_ir_data':
>> include/uapi/linux/byteorder/big_endian.h:39:42: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
39 | #define __be64_to_cpu(x) ((__force __u64)(__be64)(x))
| ^
include/linux/byteorder/generic.h:93:21: note: in expansion of macro '__be64_to_cpu'
93 | #define be64_to_cpu __be64_to_cpu
| ^~~~~~~~~~~~~
drivers/media/rc/imon_raw.c:32:20: note: in expansion of macro 'be64_to_cpu'
32 | u64 data = be64_to_cpu(imon->ir_buf);
| ^~~~~~~~~~~
vim +39 include/uapi/linux/byteorder/big_endian.h
5921e6f8809b16 David Howells 2012-10-13 15
5921e6f8809b16 David Howells 2012-10-13 16 #define __constant_htonl(x) ((__force __be32)(__u32)(x))
5921e6f8809b16 David Howells 2012-10-13 17 #define __constant_ntohl(x) ((__force __u32)(__be32)(x))
5921e6f8809b16 David Howells 2012-10-13 18 #define __constant_htons(x) ((__force __be16)(__u16)(x))
5921e6f8809b16 David Howells 2012-10-13 19 #define __constant_ntohs(x) ((__force __u16)(__be16)(x))
5921e6f8809b16 David Howells 2012-10-13 20 #define __constant_cpu_to_le64(x) ((__force __le64)___constant_swab64((x)))
5921e6f8809b16 David Howells 2012-10-13 21 #define __constant_le64_to_cpu(x) ___constant_swab64((__force __u64)(__le64)(x))
5921e6f8809b16 David Howells 2012-10-13 22 #define __constant_cpu_to_le32(x) ((__force __le32)___constant_swab32((x)))
5921e6f8809b16 David Howells 2012-10-13 23 #define __constant_le32_to_cpu(x) ___constant_swab32((__force __u32)(__le32)(x))
5921e6f8809b16 David Howells 2012-10-13 24 #define __constant_cpu_to_le16(x) ((__force __le16)___constant_swab16((x)))
5921e6f8809b16 David Howells 2012-10-13 25 #define __constant_le16_to_cpu(x) ___constant_swab16((__force __u16)(__le16)(x))
5921e6f8809b16 David Howells 2012-10-13 26 #define __constant_cpu_to_be64(x) ((__force __be64)(__u64)(x))
5921e6f8809b16 David Howells 2012-10-13 27 #define __constant_be64_to_cpu(x) ((__force __u64)(__be64)(x))
5921e6f8809b16 David Howells 2012-10-13 28 #define __constant_cpu_to_be32(x) ((__force __be32)(__u32)(x))
5921e6f8809b16 David Howells 2012-10-13 29 #define __constant_be32_to_cpu(x) ((__force __u32)(__be32)(x))
5921e6f8809b16 David Howells 2012-10-13 30 #define __constant_cpu_to_be16(x) ((__force __be16)(__u16)(x))
5921e6f8809b16 David Howells 2012-10-13 31 #define __constant_be16_to_cpu(x) ((__force __u16)(__be16)(x))
5921e6f8809b16 David Howells 2012-10-13 32 #define __cpu_to_le64(x) ((__force __le64)__swab64((x)))
5921e6f8809b16 David Howells 2012-10-13 33 #define __le64_to_cpu(x) __swab64((__force __u64)(__le64)(x))
5921e6f8809b16 David Howells 2012-10-13 34 #define __cpu_to_le32(x) ((__force __le32)__swab32((x)))
5921e6f8809b16 David Howells 2012-10-13 35 #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
5921e6f8809b16 David Howells 2012-10-13 36 #define __cpu_to_le16(x) ((__force __le16)__swab16((x)))
5921e6f8809b16 David Howells 2012-10-13 37 #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
5921e6f8809b16 David Howells 2012-10-13 38 #define __cpu_to_be64(x) ((__force __be64)(__u64)(x))
5921e6f8809b16 David Howells 2012-10-13 @39 #define __be64_to_cpu(x) ((__force __u64)(__be64)(x))
5921e6f8809b16 David Howells 2012-10-13 40 #define __cpu_to_be32(x) ((__force __be32)(__u32)(x))
5921e6f8809b16 David Howells 2012-10-13 41 #define __be32_to_cpu(x) ((__force __u32)(__be32)(x))
5921e6f8809b16 David Howells 2012-10-13 42 #define __cpu_to_be16(x) ((__force __be16)(__u16)(x))
5921e6f8809b16 David Howells 2012-10-13 43 #define __be16_to_cpu(x) ((__force __u16)(__be16)(x))
5921e6f8809b16 David Howells 2012-10-13 44
Hi Oliver,
I love your patch! Perhaps something to improve:
[auto build test WARNING on media-tree/master]
[also build test WARNING on v5.18-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Oliver-Neukum/imon_raw-respect-DMA-coherency/20220512-210422
base: git://linuxtv.org/media_tree.git master
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20220513/202205130246.Kh3xxrsg-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/5e1a1b1e9c8288033f5f1f1d70a3d7506114fad3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Oliver-Neukum/imon_raw-respect-DMA-coherency/20220512-210422
git checkout 5e1a1b1e9c8288033f5f1f1d70a3d7506114fad3
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/media/rc/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/swab.h:5,
from include/uapi/linux/byteorder/little_endian.h:14,
from include/linux/byteorder/little_endian.h:5,
from arch/arm/include/uapi/asm/byteorder.h:22,
from include/asm-generic/bitops/le.h:6,
from arch/arm/include/asm/bitops.h:267,
from include/linux/bitops.h:33,
from include/linux/log2.h:12,
from include/asm-generic/div64.h:55,
from arch/arm/include/asm/div64.h:107,
from include/linux/math.h:6,
from include/linux/math64.h:6,
from include/linux/time.h:6,
from include/linux/stat.h:19,
from include/linux/module.h:13,
from drivers/media/rc/imon_raw.c:5:
drivers/media/rc/imon_raw.c: In function 'imon_ir_data':
>> include/uapi/linux/byteorder/little_endian.h:39:50: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
39 | #define __be64_to_cpu(x) __swab64((__force __u64)(__be64)(x))
| ^
include/uapi/linux/swab.h:128:54: note: in definition of macro '__swab64'
128 | #define __swab64(x) (__u64)__builtin_bswap64((__u64)(x))
| ^
include/linux/byteorder/generic.h:93:21: note: in expansion of macro '__be64_to_cpu'
93 | #define be64_to_cpu __be64_to_cpu
| ^~~~~~~~~~~~~
drivers/media/rc/imon_raw.c:32:20: note: in expansion of macro 'be64_to_cpu'
32 | u64 data = be64_to_cpu(imon->ir_buf);
| ^~~~~~~~~~~
vim +39 include/uapi/linux/byteorder/little_endian.h
5921e6f8809b16 David Howells 2012-10-13 15
5921e6f8809b16 David Howells 2012-10-13 16 #define __constant_htonl(x) ((__force __be32)___constant_swab32((x)))
5921e6f8809b16 David Howells 2012-10-13 17 #define __constant_ntohl(x) ___constant_swab32((__force __be32)(x))
5921e6f8809b16 David Howells 2012-10-13 18 #define __constant_htons(x) ((__force __be16)___constant_swab16((x)))
5921e6f8809b16 David Howells 2012-10-13 19 #define __constant_ntohs(x) ___constant_swab16((__force __be16)(x))
5921e6f8809b16 David Howells 2012-10-13 20 #define __constant_cpu_to_le64(x) ((__force __le64)(__u64)(x))
5921e6f8809b16 David Howells 2012-10-13 21 #define __constant_le64_to_cpu(x) ((__force __u64)(__le64)(x))
5921e6f8809b16 David Howells 2012-10-13 22 #define __constant_cpu_to_le32(x) ((__force __le32)(__u32)(x))
5921e6f8809b16 David Howells 2012-10-13 23 #define __constant_le32_to_cpu(x) ((__force __u32)(__le32)(x))
5921e6f8809b16 David Howells 2012-10-13 24 #define __constant_cpu_to_le16(x) ((__force __le16)(__u16)(x))
5921e6f8809b16 David Howells 2012-10-13 25 #define __constant_le16_to_cpu(x) ((__force __u16)(__le16)(x))
5921e6f8809b16 David Howells 2012-10-13 26 #define __constant_cpu_to_be64(x) ((__force __be64)___constant_swab64((x)))
5921e6f8809b16 David Howells 2012-10-13 27 #define __constant_be64_to_cpu(x) ___constant_swab64((__force __u64)(__be64)(x))
5921e6f8809b16 David Howells 2012-10-13 28 #define __constant_cpu_to_be32(x) ((__force __be32)___constant_swab32((x)))
5921e6f8809b16 David Howells 2012-10-13 29 #define __constant_be32_to_cpu(x) ___constant_swab32((__force __u32)(__be32)(x))
5921e6f8809b16 David Howells 2012-10-13 30 #define __constant_cpu_to_be16(x) ((__force __be16)___constant_swab16((x)))
5921e6f8809b16 David Howells 2012-10-13 31 #define __constant_be16_to_cpu(x) ___constant_swab16((__force __u16)(__be16)(x))
5921e6f8809b16 David Howells 2012-10-13 32 #define __cpu_to_le64(x) ((__force __le64)(__u64)(x))
5921e6f8809b16 David Howells 2012-10-13 33 #define __le64_to_cpu(x) ((__force __u64)(__le64)(x))
5921e6f8809b16 David Howells 2012-10-13 34 #define __cpu_to_le32(x) ((__force __le32)(__u32)(x))
5921e6f8809b16 David Howells 2012-10-13 35 #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
5921e6f8809b16 David Howells 2012-10-13 36 #define __cpu_to_le16(x) ((__force __le16)(__u16)(x))
5921e6f8809b16 David Howells 2012-10-13 37 #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
5921e6f8809b16 David Howells 2012-10-13 38 #define __cpu_to_be64(x) ((__force __be64)__swab64((x)))
5921e6f8809b16 David Howells 2012-10-13 @39 #define __be64_to_cpu(x) __swab64((__force __u64)(__be64)(x))
5921e6f8809b16 David Howells 2012-10-13 40 #define __cpu_to_be32(x) ((__force __be32)__swab32((x)))
5921e6f8809b16 David Howells 2012-10-13 41 #define __be32_to_cpu(x) __swab32((__force __u32)(__be32)(x))
5921e6f8809b16 David Howells 2012-10-13 42 #define __cpu_to_be16(x) ((__force __be16)__swab16((x)))
5921e6f8809b16 David Howells 2012-10-13 43 #define __be16_to_cpu(x) __swab16((__force __u16)(__be16)(x))
5921e6f8809b16 David Howells 2012-10-13 44
Hi Oliver,
I love your patch! Perhaps something to improve:
[auto build test WARNING on media-tree/master]
[also build test WARNING on v5.18-rc6 next-20220512]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Oliver-Neukum/imon_raw-respect-DMA-coherency/20220512-210422
base: git://linuxtv.org/media_tree.git master
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20220513/202205131229.FeABo9N9-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 11.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-dirty
# https://github.com/intel-lab-lkp/linux/commit/5e1a1b1e9c8288033f5f1f1d70a3d7506114fad3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Oliver-Neukum/imon_raw-respect-DMA-coherency/20220512-210422
git checkout 5e1a1b1e9c8288033f5f1f1d70a3d7506114fad3
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sh SHELL=/bin/bash drivers/media/rc/ drivers/platform/mellanox/ net/rxrpc/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/media/rc/imon_raw.c:32:20: sparse: sparse: cast to restricted __be64
>> drivers/media/rc/imon_raw.c:32:20: sparse: sparse: cast to restricted __be64
>> drivers/media/rc/imon_raw.c:32:20: sparse: sparse: cast to restricted __be64
>> drivers/media/rc/imon_raw.c:32:20: sparse: sparse: cast to restricted __be64
>> drivers/media/rc/imon_raw.c:32:20: sparse: sparse: cast to restricted __be64
>> drivers/media/rc/imon_raw.c:32:20: sparse: sparse: cast to restricted __be64
>> drivers/media/rc/imon_raw.c:32:20: sparse: sparse: cast to restricted __be64
>> drivers/media/rc/imon_raw.c:32:20: sparse: sparse: cast to restricted __be64
>> drivers/media/rc/imon_raw.c:32:20: sparse: sparse: cast to restricted __be64
>> drivers/media/rc/imon_raw.c:32:20: sparse: sparse: cast to restricted __be64
>> drivers/media/rc/imon_raw.c:32:20: sparse: sparse: non size-preserving pointer to integer cast
vim +32 drivers/media/rc/imon_raw.c
8a4e8f8dfc6994 Sean Young 2018-01-05 20
8a4e8f8dfc6994 Sean Young 2018-01-05 21 /*
8d023a5787775c Sean Young 2018-10-18 22 * The first 5 bytes of data represent IR pulse or space. Each bit, starting
8d023a5787775c Sean Young 2018-10-18 23 * from highest bit in the first byte, represents 250µs of data. It is 1
8d023a5787775c Sean Young 2018-10-18 24 * for space and 0 for pulse.
8d023a5787775c Sean Young 2018-10-18 25 *
8d023a5787775c Sean Young 2018-10-18 26 * The station sends 10 packets, and the 7th byte will be number 1 to 10, so
8d023a5787775c Sean Young 2018-10-18 27 * when we receive 10 we assume all the data has arrived.
8a4e8f8dfc6994 Sean Young 2018-01-05 28 */
8a4e8f8dfc6994 Sean Young 2018-01-05 29 static void imon_ir_data(struct imon *imon)
8a4e8f8dfc6994 Sean Young 2018-01-05 30 {
183e19f5b9ee18 Sean Young 2018-08-21 31 struct ir_raw_event rawir = {};
e70d13f7ac061d Sean Young 2019-08-09 @32 u64 data = be64_to_cpu(imon->ir_buf);
e70d13f7ac061d Sean Young 2019-08-09 33 u8 packet_no = data & 0xff;
8d023a5787775c Sean Young 2018-10-18 34 int offset = 40;
8a4e8f8dfc6994 Sean Young 2018-01-05 35 int bit;
8a4e8f8dfc6994 Sean Young 2018-01-05 36
e70d13f7ac061d Sean Young 2019-08-09 37 if (packet_no == 0xff)
e70d13f7ac061d Sean Young 2019-08-09 38 return;
e70d13f7ac061d Sean Young 2019-08-09 39
e70d13f7ac061d Sean Young 2019-08-09 40 dev_dbg(imon->dev, "data: %*ph", 8, &imon->ir_buf);
e70d13f7ac061d Sean Young 2019-08-09 41
e70d13f7ac061d Sean Young 2019-08-09 42 /*
e70d13f7ac061d Sean Young 2019-08-09 43 * Only the first 5 bytes contain IR data. Right shift so we move
e70d13f7ac061d Sean Young 2019-08-09 44 * the IR bits to the lower 40 bits.
e70d13f7ac061d Sean Young 2019-08-09 45 */
e70d13f7ac061d Sean Young 2019-08-09 46 data >>= 24;
8a4e8f8dfc6994 Sean Young 2018-01-05 47
8d023a5787775c Sean Young 2018-10-18 48 do {
e70d13f7ac061d Sean Young 2019-08-09 49 /*
e70d13f7ac061d Sean Young 2019-08-09 50 * Find highest set bit which is less or equal to offset
e70d13f7ac061d Sean Young 2019-08-09 51 *
e70d13f7ac061d Sean Young 2019-08-09 52 * offset is the bit above (base 0) where we start looking.
e70d13f7ac061d Sean Young 2019-08-09 53 *
e70d13f7ac061d Sean Young 2019-08-09 54 * data & (BIT_ULL(offset) - 1) masks off any unwanted bits,
e70d13f7ac061d Sean Young 2019-08-09 55 * so we have just bits less than offset.
e70d13f7ac061d Sean Young 2019-08-09 56 *
e70d13f7ac061d Sean Young 2019-08-09 57 * fls will tell us the highest bit set plus 1 (or 0 if no
e70d13f7ac061d Sean Young 2019-08-09 58 * bits are set).
e70d13f7ac061d Sean Young 2019-08-09 59 */
d587cdb2a5f570 Sean Young 2019-10-07 60 rawir.pulse = !rawir.pulse;
e70d13f7ac061d Sean Young 2019-08-09 61 bit = fls64(data & (BIT_ULL(offset) - 1));
8d023a5787775c Sean Young 2018-10-18 62 if (bit < offset) {
d587cdb2a5f570 Sean Young 2019-10-07 63 dev_dbg(imon->dev, "%s: %d bits",
d587cdb2a5f570 Sean Young 2019-10-07 64 rawir.pulse ? "pulse" : "space", offset - bit);
8d023a5787775c Sean Young 2018-10-18 65 rawir.duration = (offset - bit) * BIT_DURATION;
8a4e8f8dfc6994 Sean Young 2018-01-05 66 ir_raw_event_store_with_filter(imon->rcdev, &rawir);
8a4e8f8dfc6994 Sean Young 2018-01-05 67
8a4e8f8dfc6994 Sean Young 2018-01-05 68 offset = bit;
8d023a5787775c Sean Young 2018-10-18 69 }
8d023a5787775c Sean Young 2018-10-18 70
d587cdb2a5f570 Sean Young 2019-10-07 71 data = ~data;
8d023a5787775c Sean Young 2018-10-18 72 } while (offset > 0);
8a4e8f8dfc6994 Sean Young 2018-01-05 73
494fce160f2dac Sean Young 2019-08-09 74 if (packet_no == 0x0a && !imon->rcdev->idle) {
8a4e8f8dfc6994 Sean Young 2018-01-05 75 ir_raw_event_set_idle(imon->rcdev, true);
8a4e8f8dfc6994 Sean Young 2018-01-05 76 ir_raw_event_handle(imon->rcdev);
8a4e8f8dfc6994 Sean Young 2018-01-05 77 }
8a4e8f8dfc6994 Sean Young 2018-01-05 78 }
8a4e8f8dfc6994 Sean Young 2018-01-05 79
On Thu, May 12, 2022 at 03:03:21PM +0200, Oliver Neukum wrote: > No buffer can be embedded inside a descriptor, not even a simple be64. > Use a separate kmalloc() This patch needs a tiny change from be64_to_cpu() to be64_to_cpup(), I've tested that change with the hardware. Applied to my tree: https://git.linuxtv.org/syoung/media_tree.git/log/?h=for-v5.20a Thanks, Sean > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > --- > drivers/media/rc/imon_raw.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/rc/imon_raw.c b/drivers/media/rc/imon_raw.c > index d41580f6e4c7..b7a0c0b34378 100644 > --- a/drivers/media/rc/imon_raw.c > +++ b/drivers/media/rc/imon_raw.c > @@ -14,7 +14,7 @@ struct imon { > struct device *dev; > struct urb *ir_urb; > struct rc_dev *rcdev; > - __be64 ir_buf; > + __be64 *ir_buf; > char phys[64]; > }; > > @@ -137,10 +137,16 @@ static int imon_probe(struct usb_interface *intf, > if (!imon->ir_urb) > return -ENOMEM; > > + imon->ir_buf = kmalloc(sizeof(__be64), GFP_KERNEL); > + if (!imon->ir_buf) { > + ret = -ENOMEM; > + goto free_urb; > + } > + > imon->dev = &intf->dev; > usb_fill_int_urb(imon->ir_urb, udev, > usb_rcvintpipe(udev, ir_ep->bEndpointAddress), > - &imon->ir_buf, sizeof(imon->ir_buf), > + imon->ir_buf, sizeof(__be64), > imon_ir_rx, imon, ir_ep->bInterval); > > rcdev = devm_rc_allocate_device(&intf->dev, RC_DRIVER_IR_RAW); > @@ -177,6 +183,7 @@ static int imon_probe(struct usb_interface *intf, > > free_urb: > usb_free_urb(imon->ir_urb); > + kfree(imon->ir_buf); > return ret; > } > > @@ -186,6 +193,7 @@ static void imon_disconnect(struct usb_interface *intf) > > usb_kill_urb(imon->ir_urb); > usb_free_urb(imon->ir_urb); > + kfree(imon->ir_buf); > } > > static const struct usb_device_id imon_table[] = { > -- > 2.35.3
On 13.05.22 18:23, Sean Young wrote: Hi! > On Thu, May 12, 2022 at 03:03:21PM +0200, Oliver Neukum wrote: >> No buffer can be embedded inside a descriptor, not even a simple be64. >> Use a separate kmalloc() > This patch needs a tiny change from be64_to_cpu() to be64_to_cpup(), I've > tested that change with the hardware. Needs? It is certainly not wrong and the subsequent logging will be in the converted order, but need Regards Oliver
On Mon, May 16, 2022 at 01:00:30PM +0200, Oliver Neukum wrote: > On 13.05.22 18:23, Sean Young wrote: > Hi! > > On Thu, May 12, 2022 at 03:03:21PM +0200, Oliver Neukum wrote: > >> No buffer can be embedded inside a descriptor, not even a simple be64. > >> Use a separate kmalloc() > > This patch needs a tiny change from be64_to_cpu() to be64_to_cpup(), I've > > tested that change with the hardware. > Needs? It is certainly not wrong and the subsequent logging will be in the > converted order, but need It certainly is wrong, and it doesn't compile without it, so yes it does need it. The kernel test robot also complained about. Thanks Sean
On 16.05.22 18:40, Sean Young wrote: > On Mon, May 16, 2022 at 01:00:30PM +0200, Oliver Neukum wrote: >> On 13.05.22 18:23, Sean Young wrote: >> Hi! >>> On Thu, May 12, 2022 at 03:03:21PM +0200, Oliver Neukum wrote: >>>> No buffer can be embedded inside a descriptor, not even a simple be64. >>>> Use a separate kmalloc() >>> This patch needs a tiny change from be64_to_cpu() to be64_to_cpup(), I've >>> tested that change with the hardware. >> Needs? It is certainly not wrong and the subsequent logging will be in the >> converted order, but need > It certainly is wrong, and it doesn't compile without it, so yes it does > need it. The kernel test robot also complained about. > > Hi, sorry, it seems I forgot a "*". You are right. Regards Oliver
Oliver, On Tue, May 17, 2022 at 09:34:49AM +0200, Oliver Neukum wrote: > On 16.05.22 18:40, Sean Young wrote: > > On Mon, May 16, 2022 at 01:00:30PM +0200, Oliver Neukum wrote: > >> On 13.05.22 18:23, Sean Young wrote: > >> Hi! > >>> On Thu, May 12, 2022 at 03:03:21PM +0200, Oliver Neukum wrote: > >>>> No buffer can be embedded inside a descriptor, not even a simple be64. > >>>> Use a separate kmalloc() > >>> This patch needs a tiny change from be64_to_cpu() to be64_to_cpup(), I've > >>> tested that change with the hardware. > >> Needs? It is certainly not wrong and the subsequent logging will be in the > >> converted order, but need > > It certainly is wrong, and it doesn't compile without it, so yes it does > > need it. The kernel test robot also complained about. > > > > > > Hi, > > sorry, it seems I forgot a "*". You are right. Thank you for your patches, they fix many important issues that I missed. Sean
On 17.05.22 09:42, Sean Young wrote: > Oliver, > > On Tue, May 17, 2022 at 09:34:49AM +0200, Oliver Neukum wrote: >> On 16.05.22 18:40, Sean Young wrote: >>> On Mon, May 16, 2022 at 01:00:30PM +0200, Oliver Neukum wrote: >>>> On 13.05.22 18:23, Sean Young wrote: >>>> Hi! >>>>> On Thu, May 12, 2022 at 03:03:21PM +0200, Oliver Neukum wrote: >>>>>> No buffer can be embedded inside a descriptor, not even a simple be64. >>>>>> Use a separate kmalloc() >>>>> This patch needs a tiny change from be64_to_cpu() to be64_to_cpup(), I've >>>>> tested that change with the hardware. >>>> Needs? It is certainly not wrong and the subsequent logging will be in the >>>> converted order, but need >>> It certainly is wrong, and it doesn't compile without it, so yes it does >>> need it. The kernel test robot also complained about. >>> >>> >> Hi, >> >> sorry, it seems I forgot a "*". You are right. > Thank you for your patches, they fix many important issues that I missed. > Hi, sorry for being a bit obnoxious about this, but there is a slight issue. This is the old code: static void imon_ir_data(struct imon *imon) { struct ir_raw_event rawir = {}; u64 data = be64_to_cpu(imon->ir_buf); u8 packet_no = data & 0xff; int offset = 40; int bit; if (packet_no == 0xff) return; dev_dbg(imon->dev, "data: %*ph", 8, &imon->ir_buf); The dev_dbg() logs the data as it is in the buffer. If you use be64_to_cpup() instead of be64_to_cpu() you reverse the buffer on a little endian CPU and hence the debug output will be changed. The actual driver code is unaffected, because the buffer is never used again, so this is not a big deal. The error is mine by changing the type of imon->ir_buf But the fix is not quite the best. Regards Oliver
On Tue, May 17, 2022 at 10:28:35AM +0200, Oliver Neukum wrote: > sorry for being a bit obnoxious about this, but there is a slight issue. Pointing out potential issues is the opposite of obnoxious :) > This is the old code: > > static void imon_ir_data(struct imon *imon) > { > struct ir_raw_event rawir = {}; > u64 data = be64_to_cpu(imon->ir_buf); > u8 packet_no = data & 0xff; > int offset = 40; > int bit; > > if (packet_no == 0xff) > return; > > dev_dbg(imon->dev, "data: %*ph", 8, &imon->ir_buf); That should be imon->ir_buf (not &imon->ir_buf) after your changes. > The dev_dbg() logs the data as it is in the buffer. If you use > be64_to_cpup() instead of be64_to_cpu() you reverse > the buffer on a little endian CPU and hence the debug > output will be changed. I'm confused. be64_to_cpup() does not do an in-place byte swap on little endian. I've just tested and the debug message still works fine (barring the issue above). > The actual driver code is unaffected, because the > buffer is never used again, so this is not a big deal. > > The error is mine by changing the type of imon->ir_buf > But the fix is not quite the best. Your change is good. Sean
diff --git a/drivers/media/rc/imon_raw.c b/drivers/media/rc/imon_raw.c index d41580f6e4c7..b7a0c0b34378 100644 --- a/drivers/media/rc/imon_raw.c +++ b/drivers/media/rc/imon_raw.c @@ -14,7 +14,7 @@ struct imon { struct device *dev; struct urb *ir_urb; struct rc_dev *rcdev; - __be64 ir_buf; + __be64 *ir_buf; char phys[64]; }; @@ -137,10 +137,16 @@ static int imon_probe(struct usb_interface *intf, if (!imon->ir_urb) return -ENOMEM; + imon->ir_buf = kmalloc(sizeof(__be64), GFP_KERNEL); + if (!imon->ir_buf) { + ret = -ENOMEM; + goto free_urb; + } + imon->dev = &intf->dev; usb_fill_int_urb(imon->ir_urb, udev, usb_rcvintpipe(udev, ir_ep->bEndpointAddress), - &imon->ir_buf, sizeof(imon->ir_buf), + imon->ir_buf, sizeof(__be64), imon_ir_rx, imon, ir_ep->bInterval); rcdev = devm_rc_allocate_device(&intf->dev, RC_DRIVER_IR_RAW); @@ -177,6 +183,7 @@ static int imon_probe(struct usb_interface *intf, free_urb: usb_free_urb(imon->ir_urb); + kfree(imon->ir_buf); return ret; } @@ -186,6 +193,7 @@ static void imon_disconnect(struct usb_interface *intf) usb_kill_urb(imon->ir_urb); usb_free_urb(imon->ir_urb); + kfree(imon->ir_buf); } static const struct usb_device_id imon_table[] = {
No buffer can be embedded inside a descriptor, not even a simple be64. Use a separate kmalloc() Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/media/rc/imon_raw.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)