2023-03-13 18:07:19

by Khadija Kamran

[permalink] [raw]
Subject: [PATCH] staging: axis-fifo: initialize timeouts in probe only

Module parameter, read_timeout, can only be set at the loading time. As
it can only be modified once, initialize read_timeout once in the probe
function.
As a result, only use read_timeout as the last argument in
wait_event_interruptible_timeout() call.

Same goes for write_timeout.

Suggested-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Khadija Kamran <[email protected]>
---
drivers/staging/axis-fifo/axis-fifo.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index dfd2b357f484..563caf155713 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -384,9 +384,7 @@ static ssize_t axis_fifo_read(struct file *f, char __user *buf,
mutex_lock(&fifo->read_lock);
ret = wait_event_interruptible_timeout(fifo->read_queue,
ioread32(fifo->base_addr + XLLF_RDFO_OFFSET),
- (read_timeout >= 0) ?
- msecs_to_jiffies(read_timeout) :
- MAX_SCHEDULE_TIMEOUT);
+ read_timeout);

if (ret <= 0) {
if (ret == 0) {
@@ -528,9 +526,7 @@ static ssize_t axis_fifo_write(struct file *f, const char __user *buf,
ret = wait_event_interruptible_timeout(fifo->write_queue,
ioread32(fifo->base_addr + XLLF_TDFV_OFFSET)
>= words_to_write,
- (write_timeout >= 0) ?
- msecs_to_jiffies(write_timeout) :
- MAX_SCHEDULE_TIMEOUT);
+ write_timeout);

if (ret <= 0) {
if (ret == 0) {
@@ -815,6 +811,16 @@ static int axis_fifo_probe(struct platform_device *pdev)
char *device_name;
int rc = 0; /* error return value */

+ if (read_timeout >= 0)
+ read_timeout = msecs_to_jiffies(read_timeout);
+ else
+ read_timeout = MAX_SCHEDULE_TIMEOUT;
+
+ if (write_timeout >= 0)
+ write_timeout = msecs_to_jiffies(write_timeout);
+ else
+ write_timeout = MAX_SCHEDULE_TIMEOUT;
+
/* ----------------------------
* init wrapper device
* ----------------------------
--
2.34.1



2023-03-13 19:00:47

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

Hi Khadija,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]

url: https://github.com/intel-lab-lkp/linux/commits/Khadija-Kamran/staging-axis-fifo-initialize-timeouts-in-probe-only/20230314-020827
patch link: https://lore.kernel.org/r/ZA9mThZ7NyRrQAMX%40khadija-virtual-machine
patch subject: [PATCH] staging: axis-fifo: initialize timeouts in probe only
config: ia64-allyesconfig (https://download.01.org/0day-ci/archive/20230314/[email protected]/config)
compiler: ia64-linux-gcc (GCC) 12.1.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/9d186f6c9f9bf467b48da3e28b0e9aa31fc3faf3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Khadija-Kamran/staging-axis-fifo-initialize-timeouts-in-probe-only/20230314-020827
git checkout 9d186f6c9f9bf467b48da3e28b0e9aa31fc3faf3
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/staging/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

In file included from include/linux/limits.h:7,
from include/linux/kernel.h:16,
from drivers/staging/axis-fifo/axis-fifo.c:17:
drivers/staging/axis-fifo/axis-fifo.c: In function 'axis_fifo_probe':
>> include/vdso/limits.h:11:25: warning: overflow in conversion from 'long int' to 'int' changes value from '9223372036854775807' to '-1' [-Woverflow]
11 | #define LONG_MAX ((long)(~0UL >> 1))
| ^
include/linux/sched.h:296:41: note: in expansion of macro 'LONG_MAX'
296 | #define MAX_SCHEDULE_TIMEOUT LONG_MAX
| ^~~~~~~~
drivers/staging/axis-fifo/axis-fifo.c:817:32: note: in expansion of macro 'MAX_SCHEDULE_TIMEOUT'
817 | read_timeout = MAX_SCHEDULE_TIMEOUT;
| ^~~~~~~~~~~~~~~~~~~~
>> include/vdso/limits.h:11:25: warning: overflow in conversion from 'long int' to 'int' changes value from '9223372036854775807' to '-1' [-Woverflow]
11 | #define LONG_MAX ((long)(~0UL >> 1))
| ^
include/linux/sched.h:296:41: note: in expansion of macro 'LONG_MAX'
296 | #define MAX_SCHEDULE_TIMEOUT LONG_MAX
| ^~~~~~~~
drivers/staging/axis-fifo/axis-fifo.c:822:33: note: in expansion of macro 'MAX_SCHEDULE_TIMEOUT'
822 | write_timeout = MAX_SCHEDULE_TIMEOUT;
| ^~~~~~~~~~~~~~~~~~~~


vim +11 include/vdso/limits.h

3e0e9f8c6e3ca9 Vincenzo Frascino 2020-03-20 4
3e0e9f8c6e3ca9 Vincenzo Frascino 2020-03-20 5 #define USHRT_MAX ((unsigned short)~0U)
3e0e9f8c6e3ca9 Vincenzo Frascino 2020-03-20 6 #define SHRT_MAX ((short)(USHRT_MAX >> 1))
3e0e9f8c6e3ca9 Vincenzo Frascino 2020-03-20 7 #define SHRT_MIN ((short)(-SHRT_MAX - 1))
3e0e9f8c6e3ca9 Vincenzo Frascino 2020-03-20 8 #define INT_MAX ((int)(~0U >> 1))
3e0e9f8c6e3ca9 Vincenzo Frascino 2020-03-20 9 #define INT_MIN (-INT_MAX - 1)
3e0e9f8c6e3ca9 Vincenzo Frascino 2020-03-20 10 #define UINT_MAX (~0U)
3e0e9f8c6e3ca9 Vincenzo Frascino 2020-03-20 @11 #define LONG_MAX ((long)(~0UL >> 1))
3e0e9f8c6e3ca9 Vincenzo Frascino 2020-03-20 12 #define LONG_MIN (-LONG_MAX - 1)
3e0e9f8c6e3ca9 Vincenzo Frascino 2020-03-20 13 #define ULONG_MAX (~0UL)
3e0e9f8c6e3ca9 Vincenzo Frascino 2020-03-20 14 #define LLONG_MAX ((long long)(~0ULL >> 1))
3e0e9f8c6e3ca9 Vincenzo Frascino 2020-03-20 15 #define LLONG_MIN (-LLONG_MAX - 1)
3e0e9f8c6e3ca9 Vincenzo Frascino 2020-03-20 16 #define ULLONG_MAX (~0ULL)
3e0e9f8c6e3ca9 Vincenzo Frascino 2020-03-20 17 #define UINTPTR_MAX ULONG_MAX
3e0e9f8c6e3ca9 Vincenzo Frascino 2020-03-20 18

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

2023-03-14 03:46:39

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

Hi Khadija,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]

url: https://github.com/intel-lab-lkp/linux/commits/Khadija-Kamran/staging-axis-fifo-initialize-timeouts-in-probe-only/20230314-020827
patch link: https://lore.kernel.org/r/ZA9mThZ7NyRrQAMX%40khadija-virtual-machine
patch subject: [PATCH] staging: axis-fifo: initialize timeouts in probe only
config: arm64-randconfig-r012-20230313 (https://download.01.org/0day-ci/archive/20230314/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/9d186f6c9f9bf467b48da3e28b0e9aa31fc3faf3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Khadija-Kamran/staging-axis-fifo-initialize-timeouts-in-probe-only/20230314-020827
git checkout 9d186f6c9f9bf467b48da3e28b0e9aa31fc3faf3
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/staging/axis-fifo/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

>> drivers/staging/axis-fifo/axis-fifo.c:817:18: warning: implicit conversion from 'long' to 'int' changes value from 9223372036854775807 to -1 [-Wconstant-conversion]
read_timeout = MAX_SCHEDULE_TIMEOUT;
~ ^~~~~~~~~~~~~~~~~~~~
include/linux/sched.h:296:31: note: expanded from macro 'MAX_SCHEDULE_TIMEOUT'
#define MAX_SCHEDULE_TIMEOUT LONG_MAX
^~~~~~~~
include/vdso/limits.h:11:19: note: expanded from macro 'LONG_MAX'
#define LONG_MAX ((long)(~0UL >> 1))
^~~~~~~~~~~~~~~~~
drivers/staging/axis-fifo/axis-fifo.c:822:19: warning: implicit conversion from 'long' to 'int' changes value from 9223372036854775807 to -1 [-Wconstant-conversion]
write_timeout = MAX_SCHEDULE_TIMEOUT;
~ ^~~~~~~~~~~~~~~~~~~~
include/linux/sched.h:296:31: note: expanded from macro 'MAX_SCHEDULE_TIMEOUT'
#define MAX_SCHEDULE_TIMEOUT LONG_MAX
^~~~~~~~
include/vdso/limits.h:11:19: note: expanded from macro 'LONG_MAX'
#define LONG_MAX ((long)(~0UL >> 1))
^~~~~~~~~~~~~~~~~
2 warnings generated.


vim +817 drivers/staging/axis-fifo/axis-fifo.c

805
806 static int axis_fifo_probe(struct platform_device *pdev)
807 {
808 struct resource *r_mem; /* IO mem resources */
809 struct device *dev = &pdev->dev; /* OS device (from device tree) */
810 struct axis_fifo *fifo = NULL;
811 char *device_name;
812 int rc = 0; /* error return value */
813
814 if (read_timeout >= 0)
815 read_timeout = msecs_to_jiffies(read_timeout);
816 else
> 817 read_timeout = MAX_SCHEDULE_TIMEOUT;
818
819 if (write_timeout >= 0)
820 write_timeout = msecs_to_jiffies(write_timeout);
821 else
822 write_timeout = MAX_SCHEDULE_TIMEOUT;
823
824 /* ----------------------------
825 * init wrapper device
826 * ----------------------------
827 */
828
829 device_name = devm_kzalloc(dev, 32, GFP_KERNEL);
830 if (!device_name)
831 return -ENOMEM;
832
833 /* allocate device wrapper memory */
834 fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
835 if (!fifo)
836 return -ENOMEM;
837
838 dev_set_drvdata(dev, fifo);
839 fifo->dt_device = dev;
840
841 init_waitqueue_head(&fifo->read_queue);
842 init_waitqueue_head(&fifo->write_queue);
843
844 mutex_init(&fifo->read_lock);
845 mutex_init(&fifo->write_lock);
846
847 /* ----------------------------
848 * init device memory space
849 * ----------------------------
850 */
851
852 /* get iospace for the device */
853 r_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
854 if (!r_mem) {
855 dev_err(fifo->dt_device, "invalid address\n");
856 rc = -ENODEV;
857 goto err_initial;
858 }
859
860 /* request physical memory */
861 fifo->base_addr = devm_ioremap_resource(fifo->dt_device, r_mem);
862 if (IS_ERR(fifo->base_addr)) {
863 rc = PTR_ERR(fifo->base_addr);
864 goto err_initial;
865 }
866
867 dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
868
869 /* create unique device name */
870 snprintf(device_name, 32, "%s_%pa", DRIVER_NAME, &r_mem->start);
871 dev_dbg(fifo->dt_device, "device name [%s]\n", device_name);
872
873 /* ----------------------------
874 * init IP
875 * ----------------------------
876 */
877
878 rc = axis_fifo_parse_dt(fifo);
879 if (rc)
880 goto err_initial;
881
882 reset_ip_core(fifo);
883
884 /* ----------------------------
885 * init device interrupts
886 * ----------------------------
887 */
888
889 /* get IRQ resource */
890 rc = platform_get_irq(pdev, 0);
891 if (rc < 0)
892 goto err_initial;
893
894 /* request IRQ */
895 fifo->irq = rc;
896 rc = devm_request_irq(fifo->dt_device, fifo->irq, &axis_fifo_irq, 0,
897 DRIVER_NAME, fifo);
898 if (rc) {
899 dev_err(fifo->dt_device, "couldn't allocate interrupt %i\n",
900 fifo->irq);
901 goto err_initial;
902 }
903
904 /* ----------------------------
905 * init char device
906 * ----------------------------
907 */
908
909 /* create character device */
910 fifo->miscdev.fops = &fops;
911 fifo->miscdev.minor = MISC_DYNAMIC_MINOR;
912 fifo->miscdev.name = device_name;
913 fifo->miscdev.groups = axis_fifo_attrs_groups;
914 fifo->miscdev.parent = dev;
915 rc = misc_register(&fifo->miscdev);
916 if (rc < 0)
917 goto err_initial;
918
919 dev_info(fifo->dt_device, "axis-fifo created at %pa mapped to 0x%pa, irq=%i\n",
920 &r_mem->start, &fifo->base_addr, fifo->irq);
921
922 return 0;
923
924 err_initial:
925 dev_set_drvdata(dev, NULL);
926 return rc;
927 }
928

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

2023-03-14 14:08:44

by Khadija Kamran

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Tue, Mar 14, 2023 at 11:45:51AM +0800, kernel test robot wrote:
> Hi Khadija,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on staging/staging-testing]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Khadija-Kamran/staging-axis-fifo-initialize-timeouts-in-probe-only/20230314-020827
> patch link: https://lore.kernel.org/r/ZA9mThZ7NyRrQAMX%40khadija-virtual-machine
> patch subject: [PATCH] staging: axis-fifo: initialize timeouts in probe only
> config: arm64-randconfig-r012-20230313 (https://download.01.org/0day-ci/archive/20230314/[email protected]/config)
> compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # install arm64 cross compiling tool for clang build
> # apt-get install binutils-aarch64-linux-gnu
> # https://github.com/intel-lab-lkp/linux/commit/9d186f6c9f9bf467b48da3e28b0e9aa31fc3faf3
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Khadija-Kamran/staging-axis-fifo-initialize-timeouts-in-probe-only/20230314-020827
> git checkout 9d186f6c9f9bf467b48da3e28b0e9aa31fc3faf3
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/staging/axis-fifo/
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <[email protected]>
> | Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/staging/axis-fifo/axis-fifo.c:817:18: warning: implicit conversion from 'long' to 'int' changes value from 9223372036854775807 to -1 [-Wconstant-conversion]
> read_timeout = MAX_SCHEDULE_TIMEOUT;
> ~ ^~~~~~~~~~~~~~~~~~~~
> include/linux/sched.h:296:31: note: expanded from macro 'MAX_SCHEDULE_TIMEOUT'
> #define MAX_SCHEDULE_TIMEOUT LONG_MAX
> ^~~~~~~~
> include/vdso/limits.h:11:19: note: expanded from macro 'LONG_MAX'
> #define LONG_MAX ((long)(~0UL >> 1))
> ^~~~~~~~~~~~~~~~~
> drivers/staging/axis-fifo/axis-fifo.c:822:19: warning: implicit conversion from 'long' to 'int' changes value from 9223372036854775807 to -1 [-Wconstant-conversion]
> write_timeout = MAX_SCHEDULE_TIMEOUT;
> ~ ^~~~~~~~~~~~~~~~~~~~
> include/linux/sched.h:296:31: note: expanded from macro 'MAX_SCHEDULE_TIMEOUT'
> #define MAX_SCHEDULE_TIMEOUT LONG_MAX
> ^~~~~~~~
> include/vdso/limits.h:11:19: note: expanded from macro 'LONG_MAX'
> #define LONG_MAX ((long)(~0UL >> 1))
> ^~~~~~~~~~~~~~~~~
> 2 warnings generated.
>

Hi everyone!
Kindly let me know if I should look into these warnings.
Thank you!

Regards,
Khadija


>
> vim +817 drivers/staging/axis-fifo/axis-fifo.c
>
> 805
> 806 static int axis_fifo_probe(struct platform_device *pdev)
> 807 {
> 808 struct resource *r_mem; /* IO mem resources */
> 809 struct device *dev = &pdev->dev; /* OS device (from device tree) */
> 810 struct axis_fifo *fifo = NULL;
> 811 char *device_name;
> 812 int rc = 0; /* error return value */
> 813
> 814 if (read_timeout >= 0)
> 815 read_timeout = msecs_to_jiffies(read_timeout);
> 816 else
> > 817 read_timeout = MAX_SCHEDULE_TIMEOUT;
> 818
> 819 if (write_timeout >= 0)
> 820 write_timeout = msecs_to_jiffies(write_timeout);
> 821 else
> 822 write_timeout = MAX_SCHEDULE_TIMEOUT;
> 823
> 824 /* ----------------------------
> 825 * init wrapper device
> 826 * ----------------------------
> 827 */
> 828
> 829 device_name = devm_kzalloc(dev, 32, GFP_KERNEL);
> 830 if (!device_name)
> 831 return -ENOMEM;
> 832
> 833 /* allocate device wrapper memory */
> 834 fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
> 835 if (!fifo)
> 836 return -ENOMEM;
> 837
> 838 dev_set_drvdata(dev, fifo);
> 839 fifo->dt_device = dev;
> 840
> 841 init_waitqueue_head(&fifo->read_queue);
> 842 init_waitqueue_head(&fifo->write_queue);
> 843
> 844 mutex_init(&fifo->read_lock);
> 845 mutex_init(&fifo->write_lock);
> 846
> 847 /* ----------------------------
> 848 * init device memory space
> 849 * ----------------------------
> 850 */
> 851
> 852 /* get iospace for the device */
> 853 r_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 854 if (!r_mem) {
> 855 dev_err(fifo->dt_device, "invalid address\n");
> 856 rc = -ENODEV;
> 857 goto err_initial;
> 858 }
> 859
> 860 /* request physical memory */
> 861 fifo->base_addr = devm_ioremap_resource(fifo->dt_device, r_mem);
> 862 if (IS_ERR(fifo->base_addr)) {
> 863 rc = PTR_ERR(fifo->base_addr);
> 864 goto err_initial;
> 865 }
> 866
> 867 dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
> 868
> 869 /* create unique device name */
> 870 snprintf(device_name, 32, "%s_%pa", DRIVER_NAME, &r_mem->start);
> 871 dev_dbg(fifo->dt_device, "device name [%s]\n", device_name);
> 872
> 873 /* ----------------------------
> 874 * init IP
> 875 * ----------------------------
> 876 */
> 877
> 878 rc = axis_fifo_parse_dt(fifo);
> 879 if (rc)
> 880 goto err_initial;
> 881
> 882 reset_ip_core(fifo);
> 883
> 884 /* ----------------------------
> 885 * init device interrupts
> 886 * ----------------------------
> 887 */
> 888
> 889 /* get IRQ resource */
> 890 rc = platform_get_irq(pdev, 0);
> 891 if (rc < 0)
> 892 goto err_initial;
> 893
> 894 /* request IRQ */
> 895 fifo->irq = rc;
> 896 rc = devm_request_irq(fifo->dt_device, fifo->irq, &axis_fifo_irq, 0,
> 897 DRIVER_NAME, fifo);
> 898 if (rc) {
> 899 dev_err(fifo->dt_device, "couldn't allocate interrupt %i\n",
> 900 fifo->irq);
> 901 goto err_initial;
> 902 }
> 903
> 904 /* ----------------------------
> 905 * init char device
> 906 * ----------------------------
> 907 */
> 908
> 909 /* create character device */
> 910 fifo->miscdev.fops = &fops;
> 911 fifo->miscdev.minor = MISC_DYNAMIC_MINOR;
> 912 fifo->miscdev.name = device_name;
> 913 fifo->miscdev.groups = axis_fifo_attrs_groups;
> 914 fifo->miscdev.parent = dev;
> 915 rc = misc_register(&fifo->miscdev);
> 916 if (rc < 0)
> 917 goto err_initial;
> 918
> 919 dev_info(fifo->dt_device, "axis-fifo created at %pa mapped to 0x%pa, irq=%i\n",
> 920 &r_mem->start, &fifo->base_addr, fifo->irq);
> 921
> 922 return 0;
> 923
> 924 err_initial:
> 925 dev_set_drvdata(dev, NULL);
> 926 return rc;
> 927 }
> 928
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests

2023-03-14 14:42:33

by Nathan Chancellor

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

Hi Khadija,

On Tue, Mar 14, 2023 at 07:08:31PM +0500, Khadija Kamran wrote:
> On Tue, Mar 14, 2023 at 11:45:51AM +0800, kernel test robot wrote:
> > Hi Khadija,
> >
> > Thank you for the patch! Perhaps something to improve:
> >
> > [auto build test WARNING on staging/staging-testing]
> >
> > url: https://github.com/intel-lab-lkp/linux/commits/Khadija-Kamran/staging-axis-fifo-initialize-timeouts-in-probe-only/20230314-020827
> > patch link: https://lore.kernel.org/r/ZA9mThZ7NyRrQAMX%40khadija-virtual-machine
> > patch subject: [PATCH] staging: axis-fifo: initialize timeouts in probe only
> > config: arm64-randconfig-r012-20230313 (https://download.01.org/0day-ci/archive/20230314/[email protected]/config)
> > compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
> > reproduce (this is a W=1 build):
> > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > # install arm64 cross compiling tool for clang build
> > # apt-get install binutils-aarch64-linux-gnu
> > # https://github.com/intel-lab-lkp/linux/commit/9d186f6c9f9bf467b48da3e28b0e9aa31fc3faf3
> > git remote add linux-review https://github.com/intel-lab-lkp/linux
> > git fetch --no-tags linux-review Khadija-Kamran/staging-axis-fifo-initialize-timeouts-in-probe-only/20230314-020827
> > git checkout 9d186f6c9f9bf467b48da3e28b0e9aa31fc3faf3
> > # save the config file
> > mkdir build_dir && cp config build_dir/.config
> > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
> > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/staging/axis-fifo/
> >
> > If you fix the issue, kindly add following tag where applicable
> > | Reported-by: kernel test robot <[email protected]>
> > | Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> >
> > All warnings (new ones prefixed by >>):
> >
> > >> drivers/staging/axis-fifo/axis-fifo.c:817:18: warning: implicit conversion from 'long' to 'int' changes value from 9223372036854775807 to -1 [-Wconstant-conversion]
> > read_timeout = MAX_SCHEDULE_TIMEOUT;
> > ~ ^~~~~~~~~~~~~~~~~~~~
> > include/linux/sched.h:296:31: note: expanded from macro 'MAX_SCHEDULE_TIMEOUT'
> > #define MAX_SCHEDULE_TIMEOUT LONG_MAX
> > ^~~~~~~~
> > include/vdso/limits.h:11:19: note: expanded from macro 'LONG_MAX'
> > #define LONG_MAX ((long)(~0UL >> 1))
> > ^~~~~~~~~~~~~~~~~
> > drivers/staging/axis-fifo/axis-fifo.c:822:19: warning: implicit conversion from 'long' to 'int' changes value from 9223372036854775807 to -1 [-Wconstant-conversion]
> > write_timeout = MAX_SCHEDULE_TIMEOUT;
> > ~ ^~~~~~~~~~~~~~~~~~~~
> > include/linux/sched.h:296:31: note: expanded from macro 'MAX_SCHEDULE_TIMEOUT'
> > #define MAX_SCHEDULE_TIMEOUT LONG_MAX
> > ^~~~~~~~
> > include/vdso/limits.h:11:19: note: expanded from macro 'LONG_MAX'
> > #define LONG_MAX ((long)(~0UL >> 1))
> > ^~~~~~~~~~~~~~~~~
> > 2 warnings generated.
> >
>
> Hi everyone!
> Kindly let me know if I should look into these warnings.
> Thank you!

You should always avoid introducing new warnings whenever possible. In
this case, it appears that read_timeout and write_timeout should be
changed from 'int' to 'long' to account for the fact that
MAX_SCHEDULE_TIMEOUT is being assigned to it directly now, versus being
passed as a parameter to wait_event_interruptible_timeout(), which
assigned it to 'long' anyways.

If you have any other questions or need further help, let me know :)

Cheers,
Nathan

> > vim +817 drivers/staging/axis-fifo/axis-fifo.c
> >
> > 805
> > 806 static int axis_fifo_probe(struct platform_device *pdev)
> > 807 {
> > 808 struct resource *r_mem; /* IO mem resources */
> > 809 struct device *dev = &pdev->dev; /* OS device (from device tree) */
> > 810 struct axis_fifo *fifo = NULL;
> > 811 char *device_name;
> > 812 int rc = 0; /* error return value */
> > 813
> > 814 if (read_timeout >= 0)
> > 815 read_timeout = msecs_to_jiffies(read_timeout);
> > 816 else
> > > 817 read_timeout = MAX_SCHEDULE_TIMEOUT;
> > 818
> > 819 if (write_timeout >= 0)
> > 820 write_timeout = msecs_to_jiffies(write_timeout);
> > 821 else
> > 822 write_timeout = MAX_SCHEDULE_TIMEOUT;
> > 823
> > 824 /* ----------------------------
> > 825 * init wrapper device
> > 826 * ----------------------------
> > 827 */
> > 828
> > 829 device_name = devm_kzalloc(dev, 32, GFP_KERNEL);
> > 830 if (!device_name)
> > 831 return -ENOMEM;
> > 832
> > 833 /* allocate device wrapper memory */
> > 834 fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
> > 835 if (!fifo)
> > 836 return -ENOMEM;
> > 837
> > 838 dev_set_drvdata(dev, fifo);
> > 839 fifo->dt_device = dev;
> > 840
> > 841 init_waitqueue_head(&fifo->read_queue);
> > 842 init_waitqueue_head(&fifo->write_queue);
> > 843
> > 844 mutex_init(&fifo->read_lock);
> > 845 mutex_init(&fifo->write_lock);
> > 846
> > 847 /* ----------------------------
> > 848 * init device memory space
> > 849 * ----------------------------
> > 850 */
> > 851
> > 852 /* get iospace for the device */
> > 853 r_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > 854 if (!r_mem) {
> > 855 dev_err(fifo->dt_device, "invalid address\n");
> > 856 rc = -ENODEV;
> > 857 goto err_initial;
> > 858 }
> > 859
> > 860 /* request physical memory */
> > 861 fifo->base_addr = devm_ioremap_resource(fifo->dt_device, r_mem);
> > 862 if (IS_ERR(fifo->base_addr)) {
> > 863 rc = PTR_ERR(fifo->base_addr);
> > 864 goto err_initial;
> > 865 }
> > 866
> > 867 dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
> > 868
> > 869 /* create unique device name */
> > 870 snprintf(device_name, 32, "%s_%pa", DRIVER_NAME, &r_mem->start);
> > 871 dev_dbg(fifo->dt_device, "device name [%s]\n", device_name);
> > 872
> > 873 /* ----------------------------
> > 874 * init IP
> > 875 * ----------------------------
> > 876 */
> > 877
> > 878 rc = axis_fifo_parse_dt(fifo);
> > 879 if (rc)
> > 880 goto err_initial;
> > 881
> > 882 reset_ip_core(fifo);
> > 883
> > 884 /* ----------------------------
> > 885 * init device interrupts
> > 886 * ----------------------------
> > 887 */
> > 888
> > 889 /* get IRQ resource */
> > 890 rc = platform_get_irq(pdev, 0);
> > 891 if (rc < 0)
> > 892 goto err_initial;
> > 893
> > 894 /* request IRQ */
> > 895 fifo->irq = rc;
> > 896 rc = devm_request_irq(fifo->dt_device, fifo->irq, &axis_fifo_irq, 0,
> > 897 DRIVER_NAME, fifo);
> > 898 if (rc) {
> > 899 dev_err(fifo->dt_device, "couldn't allocate interrupt %i\n",
> > 900 fifo->irq);
> > 901 goto err_initial;
> > 902 }
> > 903
> > 904 /* ----------------------------
> > 905 * init char device
> > 906 * ----------------------------
> > 907 */
> > 908
> > 909 /* create character device */
> > 910 fifo->miscdev.fops = &fops;
> > 911 fifo->miscdev.minor = MISC_DYNAMIC_MINOR;
> > 912 fifo->miscdev.name = device_name;
> > 913 fifo->miscdev.groups = axis_fifo_attrs_groups;
> > 914 fifo->miscdev.parent = dev;
> > 915 rc = misc_register(&fifo->miscdev);
> > 916 if (rc < 0)
> > 917 goto err_initial;
> > 918
> > 919 dev_info(fifo->dt_device, "axis-fifo created at %pa mapped to 0x%pa, irq=%i\n",
> > 920 &r_mem->start, &fifo->base_addr, fifo->irq);
> > 921
> > 922 return 0;
> > 923
> > 924 err_initial:
> > 925 dev_set_drvdata(dev, NULL);
> > 926 return rc;
> > 927 }
> > 928
> >
> > --
> > 0-DAY CI Kernel Test Service
> > https://github.com/intel/lkp-tests
>

2023-03-14 15:15:23

by Alison Schofield

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Tue, Mar 14, 2023 at 07:42:07AM -0700, Nathan Chancellor wrote:
> Hi Khadija,
>
> On Tue, Mar 14, 2023 at 07:08:31PM +0500, Khadija Kamran wrote:
> > On Tue, Mar 14, 2023 at 11:45:51AM +0800, kernel test robot wrote:
> > > Hi Khadija,
> > >
> > > Thank you for the patch! Perhaps something to improve:
> > >
> > > [auto build test WARNING on staging/staging-testing]
> > >
> > > url: https://github.com/intel-lab-lkp/linux/commits/Khadija-Kamran/staging-axis-fifo-initialize-timeouts-in-probe-only/20230314-020827
> > > patch link: https://lore.kernel.org/r/ZA9mThZ7NyRrQAMX%40khadija-virtual-machine
> > > patch subject: [PATCH] staging: axis-fifo: initialize timeouts in probe only
> > > config: arm64-randconfig-r012-20230313 (https://download.01.org/0day-ci/archive/20230314/[email protected]/config)
> > > compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
> > > reproduce (this is a W=1 build):
> > > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > > chmod +x ~/bin/make.cross
> > > # install arm64 cross compiling tool for clang build
> > > # apt-get install binutils-aarch64-linux-gnu
> > > # https://github.com/intel-lab-lkp/linux/commit/9d186f6c9f9bf467b48da3e28b0e9aa31fc3faf3
> > > git remote add linux-review https://github.com/intel-lab-lkp/linux
> > > git fetch --no-tags linux-review Khadija-Kamran/staging-axis-fifo-initialize-timeouts-in-probe-only/20230314-020827
> > > git checkout 9d186f6c9f9bf467b48da3e28b0e9aa31fc3faf3
> > > # save the config file
> > > mkdir build_dir && cp config build_dir/.config
> > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
> > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/staging/axis-fifo/
> > >
> > > If you fix the issue, kindly add following tag where applicable
> > > | Reported-by: kernel test robot <[email protected]>
> > > | Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > >
> > > All warnings (new ones prefixed by >>):
> > >
> > > >> drivers/staging/axis-fifo/axis-fifo.c:817:18: warning: implicit conversion from 'long' to 'int' changes value from 9223372036854775807 to -1 [-Wconstant-conversion]
> > > read_timeout = MAX_SCHEDULE_TIMEOUT;
> > > ~ ^~~~~~~~~~~~~~~~~~~~
> > > include/linux/sched.h:296:31: note: expanded from macro 'MAX_SCHEDULE_TIMEOUT'
> > > #define MAX_SCHEDULE_TIMEOUT LONG_MAX
> > > ^~~~~~~~
> > > include/vdso/limits.h:11:19: note: expanded from macro 'LONG_MAX'
> > > #define LONG_MAX ((long)(~0UL >> 1))
> > > ^~~~~~~~~~~~~~~~~
> > > drivers/staging/axis-fifo/axis-fifo.c:822:19: warning: implicit conversion from 'long' to 'int' changes value from 9223372036854775807 to -1 [-Wconstant-conversion]
> > > write_timeout = MAX_SCHEDULE_TIMEOUT;
> > > ~ ^~~~~~~~~~~~~~~~~~~~
> > > include/linux/sched.h:296:31: note: expanded from macro 'MAX_SCHEDULE_TIMEOUT'
> > > #define MAX_SCHEDULE_TIMEOUT LONG_MAX
> > > ^~~~~~~~
> > > include/vdso/limits.h:11:19: note: expanded from macro 'LONG_MAX'
> > > #define LONG_MAX ((long)(~0UL >> 1))
> > > ^~~~~~~~~~~~~~~~~
> > > 2 warnings generated.
> > >
> >
> > Hi everyone!
> > Kindly let me know if I should look into these warnings.
> > Thank you!
>
> You should always avoid introducing new warnings whenever possible. In
> this case, it appears that read_timeout and write_timeout should be
> changed from 'int' to 'long' to account for the fact that
> MAX_SCHEDULE_TIMEOUT is being assigned to it directly now, versus being
> passed as a parameter to wait_event_interruptible_timeout(), which
> assigned it to 'long' anyways.
>
> If you have any other questions or need further help, let me know :)
>
> Cheers,
> Nathan

Hi Khadaji,

Echoing Nathan's feedback - yes, you need to correct these.

Do you see these warnings when you compiled? If yes, then your path to
fixing them is easier. Make sure it recompiles with no warnings.

If you don't see these warnings, then you need to follow the steps to
reproduce in the lkp email. You have to be able to 'see the warning'
make a change and 'see it go away'.

Alison

>
> > > vim +817 drivers/staging/axis-fifo/axis-fifo.c
> > >
> > > 805
> > > 806 static int axis_fifo_probe(struct platform_device *pdev)
> > > 807 {
> > > 808 struct resource *r_mem; /* IO mem resources */
> > > 809 struct device *dev = &pdev->dev; /* OS device (from device tree) */
> > > 810 struct axis_fifo *fifo = NULL;
> > > 811 char *device_name;
> > > 812 int rc = 0; /* error return value */
> > > 813
> > > 814 if (read_timeout >= 0)
> > > 815 read_timeout = msecs_to_jiffies(read_timeout);
> > > 816 else
> > > > 817 read_timeout = MAX_SCHEDULE_TIMEOUT;
> > > 818
> > > 819 if (write_timeout >= 0)
> > > 820 write_timeout = msecs_to_jiffies(write_timeout);
> > > 821 else
> > > 822 write_timeout = MAX_SCHEDULE_TIMEOUT;
> > > 823
> > > 824 /* ----------------------------
> > > 825 * init wrapper device
> > > 826 * ----------------------------
> > > 827 */
> > > 828
> > > 829 device_name = devm_kzalloc(dev, 32, GFP_KERNEL);
> > > 830 if (!device_name)
> > > 831 return -ENOMEM;
> > > 832
> > > 833 /* allocate device wrapper memory */
> > > 834 fifo = devm_kzalloc(dev, sizeof(*fifo), GFP_KERNEL);
> > > 835 if (!fifo)
> > > 836 return -ENOMEM;
> > > 837
> > > 838 dev_set_drvdata(dev, fifo);
> > > 839 fifo->dt_device = dev;
> > > 840
> > > 841 init_waitqueue_head(&fifo->read_queue);
> > > 842 init_waitqueue_head(&fifo->write_queue);
> > > 843
> > > 844 mutex_init(&fifo->read_lock);
> > > 845 mutex_init(&fifo->write_lock);
> > > 846
> > > 847 /* ----------------------------
> > > 848 * init device memory space
> > > 849 * ----------------------------
> > > 850 */
> > > 851
> > > 852 /* get iospace for the device */
> > > 853 r_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > 854 if (!r_mem) {
> > > 855 dev_err(fifo->dt_device, "invalid address\n");
> > > 856 rc = -ENODEV;
> > > 857 goto err_initial;
> > > 858 }
> > > 859
> > > 860 /* request physical memory */
> > > 861 fifo->base_addr = devm_ioremap_resource(fifo->dt_device, r_mem);
> > > 862 if (IS_ERR(fifo->base_addr)) {
> > > 863 rc = PTR_ERR(fifo->base_addr);
> > > 864 goto err_initial;
> > > 865 }
> > > 866
> > > 867 dev_dbg(fifo->dt_device, "remapped memory to 0x%p\n", fifo->base_addr);
> > > 868
> > > 869 /* create unique device name */
> > > 870 snprintf(device_name, 32, "%s_%pa", DRIVER_NAME, &r_mem->start);
> > > 871 dev_dbg(fifo->dt_device, "device name [%s]\n", device_name);
> > > 872
> > > 873 /* ----------------------------
> > > 874 * init IP
> > > 875 * ----------------------------
> > > 876 */
> > > 877
> > > 878 rc = axis_fifo_parse_dt(fifo);
> > > 879 if (rc)
> > > 880 goto err_initial;
> > > 881
> > > 882 reset_ip_core(fifo);
> > > 883
> > > 884 /* ----------------------------
> > > 885 * init device interrupts
> > > 886 * ----------------------------
> > > 887 */
> > > 888
> > > 889 /* get IRQ resource */
> > > 890 rc = platform_get_irq(pdev, 0);
> > > 891 if (rc < 0)
> > > 892 goto err_initial;
> > > 893
> > > 894 /* request IRQ */
> > > 895 fifo->irq = rc;
> > > 896 rc = devm_request_irq(fifo->dt_device, fifo->irq, &axis_fifo_irq, 0,
> > > 897 DRIVER_NAME, fifo);
> > > 898 if (rc) {
> > > 899 dev_err(fifo->dt_device, "couldn't allocate interrupt %i\n",
> > > 900 fifo->irq);
> > > 901 goto err_initial;
> > > 902 }
> > > 903
> > > 904 /* ----------------------------
> > > 905 * init char device
> > > 906 * ----------------------------
> > > 907 */
> > > 908
> > > 909 /* create character device */
> > > 910 fifo->miscdev.fops = &fops;
> > > 911 fifo->miscdev.minor = MISC_DYNAMIC_MINOR;
> > > 912 fifo->miscdev.name = device_name;
> > > 913 fifo->miscdev.groups = axis_fifo_attrs_groups;
> > > 914 fifo->miscdev.parent = dev;
> > > 915 rc = misc_register(&fifo->miscdev);
> > > 916 if (rc < 0)
> > > 917 goto err_initial;
> > > 918
> > > 919 dev_info(fifo->dt_device, "axis-fifo created at %pa mapped to 0x%pa, irq=%i\n",
> > > 920 &r_mem->start, &fifo->base_addr, fifo->irq);
> > > 921
> > > 922 return 0;
> > > 923
> > > 924 err_initial:
> > > 925 dev_set_drvdata(dev, NULL);
> > > 926 return rc;
> > > 927 }
> > > 928
> > >
> > > --
> > > 0-DAY CI Kernel Test Service
> > > https://github.com/intel/lkp-tests
> >
>

2023-03-14 19:07:19

by Fabio M. De Francesco

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On marted? 14 marzo 2023 16:13:53 CET Alison Schofield wrote:
> On Tue, Mar 14, 2023 at 07:42:07AM -0700, Nathan Chancellor wrote:
> > Hi Khadija,
> >
> > On Tue, Mar 14, 2023 at 07:08:31PM +0500, Khadija Kamran wrote:
> > > On Tue, Mar 14, 2023 at 11:45:51AM +0800, kernel test robot wrote:
> > > >
> > > > [...]
> > >
> > > Hi everyone!
> > > Kindly let me know if I should look into these warnings.
> > > Thank you!
> >
> > You should always avoid introducing new warnings whenever possible. In
> > this case, it appears that read_timeout and write_timeout should be
> > changed from 'int' to 'long' to account for the fact that
> > MAX_SCHEDULE_TIMEOUT is being assigned to it directly now, versus being
> > passed as a parameter to wait_event_interruptible_timeout(), which
> > assigned it to 'long' anyways.
> >
> > If you have any other questions or need further help, let me know :)
> >
> > Cheers,
> > Nathan
>
> Hi Khadija,
>
> Echoing Nathan's feedback - yes, you need to correct these.
>
> Do you see these warnings when you compiled? If yes, then your path to
> fixing them is easier. Make sure it recompiles with no warnings.
>
> If you don't see these warnings, then you need to follow the steps to
> reproduce in the lkp email. You have to be able to 'see the warning'
> make a change and 'see it go away'.
>
> Alison
>

Hi Khadija,

I would suggest trying to recompile using "make W=1 -j...". Not sure if you'll
see the warnings (depends on your compiler, version and target architecture).
However, it's worth a try as it may save you the time it takes to go through
the steps for reproduction.

Anyway, as Nathan said, that warning is triggered by assigning 'long' to 'int'
(i.e., "MAX_SCHEDULE_TIMEOUT" is cast to 'long', while the types of
"read_timeout" and "write_timeout " are both 'int'). Obviously, these kinds of
assignments overflow wherever the representation of type 'long' uses more bits
than the type 'int'.

That implies that you should try to understand where the module's parameters
are defined and how to work with the related macros.

I hope it helps.

Fabio

P.S.: If you don't know yet what to do, take a look at the following document:
https://sysprog21.github.io/lkmpg/ (The Linux Kernel Module Programming
Guide).





2023-03-14 20:43:48

by Alison Schofield

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Mon, Mar 13, 2023 at 11:07:10PM +0500, Khadija Kamran wrote:
> Module parameter, read_timeout, can only be set at the loading time. As
> it can only be modified once, initialize read_timeout once in the probe
> function.
> As a result, only use read_timeout as the last argument in
> wait_event_interruptible_timeout() call.
>
> Same goes for write_timeout.
>
> Suggested-by: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Khadija Kamran <[email protected]>
> ---

Looks like this is [PATCH v5] and needs a changelog.


> drivers/staging/axis-fifo/axis-fifo.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
> index dfd2b357f484..563caf155713 100644
> --- a/drivers/staging/axis-fifo/axis-fifo.c
> +++ b/drivers/staging/axis-fifo/axis-fifo.c
> @@ -384,9 +384,7 @@ static ssize_t axis_fifo_read(struct file *f, char __user *buf,
> mutex_lock(&fifo->read_lock);
> ret = wait_event_interruptible_timeout(fifo->read_queue,
> ioread32(fifo->base_addr + XLLF_RDFO_OFFSET),
> - (read_timeout >= 0) ?
> - msecs_to_jiffies(read_timeout) :
> - MAX_SCHEDULE_TIMEOUT);
> + read_timeout);
>
> if (ret <= 0) {
> if (ret == 0) {
> @@ -528,9 +526,7 @@ static ssize_t axis_fifo_write(struct file *f, const char __user *buf,
> ret = wait_event_interruptible_timeout(fifo->write_queue,
> ioread32(fifo->base_addr + XLLF_TDFV_OFFSET)
> >= words_to_write,
> - (write_timeout >= 0) ?
> - msecs_to_jiffies(write_timeout) :
> - MAX_SCHEDULE_TIMEOUT);
> + write_timeout);
>
> if (ret <= 0) {
> if (ret == 0) {
> @@ -815,6 +811,16 @@ static int axis_fifo_probe(struct platform_device *pdev)
> char *device_name;
> int rc = 0; /* error return value */
>
> + if (read_timeout >= 0)
> + read_timeout = msecs_to_jiffies(read_timeout);
> + else
> + read_timeout = MAX_SCHEDULE_TIMEOUT;
> +
> + if (write_timeout >= 0)
> + write_timeout = msecs_to_jiffies(write_timeout);
> + else
> + write_timeout = MAX_SCHEDULE_TIMEOUT;
> +
> /* ----------------------------
> * init wrapper device
> * ----------------------------
> --
> 2.34.1
>
>

2023-03-14 21:31:30

by Fabio M. De Francesco

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On marted? 14 marzo 2023 21:43:40 CET Alison Schofield wrote:
> On Mon, Mar 13, 2023 at 11:07:10PM +0500, Khadija Kamran wrote:
> > Module parameter, read_timeout, can only be set at the loading time. As
> > it can only be modified once, initialize read_timeout once in the probe
> > function.
> > As a result, only use read_timeout as the last argument in
> > wait_event_interruptible_timeout() call.
> >
> > Same goes for write_timeout.
> >
> > Suggested-by: Greg Kroah-Hartman <[email protected]>
> > Signed-off-by: Khadija Kamran <[email protected]>
> > ---
>
> Looks like this is [PATCH v5] and needs a changelog.
>
Alison,

In fact, this is only the second patch that addresses Greg's suggested
refactoring.

Khadija moved from v3 of "staging: axis-fifo: remove tabs to align arguments"
to v4 of this completely independent patch. And then back to v1, because (at
the time of v4) I pointed out to her that she had started working on a project
that has a completely different purpose than the previous one.

The best course of action would have been to ask Greg to drop the previous
patches and then reset the numbering of the new job to v1. Unfortunately I did
not pay attention to how she then managed the numbering following my
observation.

What would be the best course of action at this point?

Fabio



2023-03-14 23:57:53

by Alison Schofield

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Tue, Mar 14, 2023 at 10:31:21PM +0100, Fabio wrote:
> On marted? 14 marzo 2023 21:43:40 CET Alison Schofield wrote:
> > On Mon, Mar 13, 2023 at 11:07:10PM +0500, Khadija Kamran wrote:
> > > Module parameter, read_timeout, can only be set at the loading time. As
> > > it can only be modified once, initialize read_timeout once in the probe
> > > function.
> > > As a result, only use read_timeout as the last argument in
> > > wait_event_interruptible_timeout() call.
> > >
> > > Same goes for write_timeout.
> > >
> > > Suggested-by: Greg Kroah-Hartman <[email protected]>
> > > Signed-off-by: Khadija Kamran <[email protected]>
> > > ---
> >
> > Looks like this is [PATCH v5] and needs a changelog.
> >
> Alison,
>
> In fact, this is only the second patch that addresses Greg's suggested
> refactoring.
>
> Khadija moved from v3 of "staging: axis-fifo: remove tabs to align arguments"
> to v4 of this completely independent patch. And then back to v1, because (at
> the time of v4) I pointed out to her that she had started working on a project
> that has a completely different purpose than the previous one.
>
> The best course of action would have been to ask Greg to drop the previous
> patches and then reset the numbering of the new job to v1. Unfortunately I did
> not pay attention to how she then managed the numbering following my
> observation.
>
> What would be the best course of action at this point?

My guess is that this patch gets ignored because it has a lower version
number than a previous patch.

Take the feedback given here, and rev to
[PATCH v5] staging: axis-fifo: initialize timeouts in probe only

Be sure the Changelog, below the --- explains the journey.

Changes in v5:

Changes in v4:

Changes in v3:

Changes in v2:


>
> Fabio
>
>

2023-03-15 12:34:59

by Khadija Kamran

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Tue, Mar 14, 2023 at 04:57:47PM -0700, Alison Schofield wrote:
> On Tue, Mar 14, 2023 at 10:31:21PM +0100, Fabio wrote:
> > On marted? 14 marzo 2023 21:43:40 CET Alison Schofield wrote:
> > > On Mon, Mar 13, 2023 at 11:07:10PM +0500, Khadija Kamran wrote:
> > > > Module parameter, read_timeout, can only be set at the loading time. As
> > > > it can only be modified once, initialize read_timeout once in the probe
> > > > function.
> > > > As a result, only use read_timeout as the last argument in
> > > > wait_event_interruptible_timeout() call.
> > > >
> > > > Same goes for write_timeout.
> > > >
> > > > Suggested-by: Greg Kroah-Hartman <[email protected]>
> > > > Signed-off-by: Khadija Kamran <[email protected]>
> > > > ---
> > >
> > > Looks like this is [PATCH v5] and needs a changelog.
> > >
> > Alison,
> >
> > In fact, this is only the second patch that addresses Greg's suggested
> > refactoring.
> >
> > Khadija moved from v3 of "staging: axis-fifo: remove tabs to align arguments"
> > to v4 of this completely independent patch. And then back to v1, because (at
> > the time of v4) I pointed out to her that she had started working on a project
> > that has a completely different purpose than the previous one.
> >
> > The best course of action would have been to ask Greg to drop the previous
> > patches and then reset the numbering of the new job to v1. Unfortunately I did
> > not pay attention to how she then managed the numbering following my
> > observation.
> >
> > What would be the best course of action at this point?
>
> My guess is that this patch gets ignored because it has a lower version
> number than a previous patch.
>
> Take the feedback given here, and rev to
> [PATCH v5] staging: axis-fifo: initialize timeouts in probe only
>
> Be sure the Changelog, below the --- explains the journey.
>
> Changes in v5:
>
> Changes in v4:
>
> Changes in v3:
>
> Changes in v2:
>

Hey Alison!
Based on Nathan's feedback I am trying to recompile and send a patch
without any warnings.
As suggested by Fabio, I am running "make w=1 -jX" command to see if I
get any warnings. But it is taking a lot of time, is there any way of
speeding it up?
If this doesn't work then I have to follow the steps to reproduce in lkp
mail as you said before.
After dealing with these warnings I will send a [PATCH v5], following
your instructions above.
Kindly, let me know if I am on the wrong track.
Thank you!

Regards,
Khadija

>
> >
> > Fabio
> >
> >

2023-03-15 13:14:19

by Fabio M. De Francesco

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On mercoled? 15 marzo 2023 13:32:55 CET Khadija Kamran wrote:
> On Tue, Mar 14, 2023 at 04:57:47PM -0700, Alison Schofield wrote:
> > On Tue, Mar 14, 2023 at 10:31:21PM +0100, Fabio wrote:
> > > On marted? 14 marzo 2023 21:43:40 CET Alison Schofield wrote:
> > > > On Mon, Mar 13, 2023 at 11:07:10PM +0500, Khadija Kamran wrote:
> > > > > Module parameter, read_timeout, can only be set at the loading time.
> > > > > As
> > > > > it can only be modified once, initialize read_timeout once in the
> > > > > probe
> > > > > function.
> > > > > As a result, only use read_timeout as the last argument in
> > > > > wait_event_interruptible_timeout() call.
> > > > >
> > > > > Same goes for write_timeout.
> > > > >
> > > > > Suggested-by: Greg Kroah-Hartman <[email protected]>
> > > > > Signed-off-by: Khadija Kamran <[email protected]>
> > > > > ---
> > > >
> > > > Looks like this is [PATCH v5] and needs a changelog.
> > >
> > > Alison,
> > >
> > > In fact, this is only the second patch that addresses Greg's suggested
> > > refactoring.
> > >
> > > Khadija moved from v3 of "staging: axis-fifo: remove tabs to align
> > > arguments"
> > > to v4 of this completely independent patch. And then back to v1, because
> > > (at
> > > the time of v4) I pointed out to her that she had started working on a
> > > project that has a completely different purpose than the previous one.
> > >
> > > The best course of action would have been to ask Greg to drop the
previous
> > > patches and then reset the numbering of the new job to v1. Unfortunately
I
> > > did not pay attention to how she then managed the numbering following my
> > > observation.
> > >
> > > What would be the best course of action at this point?
> >
> > My guess is that this patch gets ignored because it has a lower version
> > number than a previous patch.
> >
> > Take the feedback given here, and rev to
> > [PATCH v5] staging: axis-fifo: initialize timeouts in probe only
> >
> > Be sure the Changelog, below the --- explains the journey.
> >
> > Changes in v5:
> >
> > Changes in v4:
> >
> > Changes in v3:
>
> > Changes in v2:
> Hey Alison!

Hi Khadija,

Please put one or two blank lines between the last message you are replying
and the new you are writing (exactly as I'm doing here between "Hey Alison!"
and "Hi Khadija").

> Based on Nathan's feedback I am trying to recompile and send a patch
> without any warnings.

Great!

> As suggested by Fabio, I am running "make w=1 -jX" command to see if I
> get any warnings.

I suppose that "w=1" is a typo. The option is enabled with "W=1" (capital
case, Linux and all UNIX-like are case-sensitive).

> But it is taking a lot of time, is there any way of
> speeding it up?

What is you choice for 'X' in "-jX"?
Did you try with the exact number of logical cores?
Are you building into a VM with enough logical cores?
If you are building into a VM, did you reserve enough RAM?

Please read carefully my questions above and try to understand your
environment and reply, so that I can help you more effectively.

> If this doesn't work then I have to follow the steps to reproduce in lkp
> mail as you said before.

The steps to reproduce will take your precious time and use more resources.
Again, try to respond my questions.

> After dealing with these warnings I will send a [PATCH v5], following
> your instructions above.

Sorry for inadvertently overlooking to warn you about to send a message to
Greg and ask him to drop your first 3 + 1 patches. Now you are doing good by
following what Alison suggested: send v5 and write the log of revisions under
the three dashes (exactly how Alison explained).

> Kindly, let me know if I am on the wrong track.
> Thank you!
>

I think you are in the right track.
Let's try to speed up your builds because you'll need to build again your
kernel many, many times for future works.

Thanks,

Fabio



2023-03-15 13:35:04

by Fabio M. De Francesco

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On mercoled? 15 marzo 2023 13:32:55 CET Khadija Kamran wrote:
> On Tue, Mar 14, 2023 at 04:57:47PM -0700, Alison Schofield wrote:
> > On Tue, Mar 14, 2023 at 10:31:21PM +0100, Fabio wrote:
> > > On marted? 14 marzo 2023 21:43:40 CET Alison Schofield wrote:
> > > > On Mon, Mar 13, 2023 at 11:07:10PM +0500, Khadija Kamran wrote:
> > > > > Module parameter, read_timeout, can only be set at the loading time.
> > > > > As
> > > > > it can only be modified once, initialize read_timeout once in the
> > > > > probe
> > > > > function.
> > > > > As a result, only use read_timeout as the last argument in
> > > > > wait_event_interruptible_timeout() call.
> > > > >
> > > > > Same goes for write_timeout.
> > > > >
> > > > > Suggested-by: Greg Kroah-Hartman <[email protected]>
> > > > > Signed-off-by: Khadija Kamran <[email protected]>
> > > > > ---
> > > >
> > > > Looks like this is [PATCH v5] and needs a changelog.
> > >
> > > Alison,
> > >
> > > In fact, this is only the second patch that addresses Greg's suggested
> > > refactoring.
> > >
> > > Khadija moved from v3 of "staging: axis-fifo: remove tabs to align
> > > arguments"
> > > to v4 of this completely independent patch. And then back to v1, because
> > > (at
> > > the time of v4) I pointed out to her that she had started working on a
> > > project that has a completely different purpose than the previous one.
> > >
> > > The best course of action would have been to ask Greg to drop the
previous
> > > patches and then reset the numbering of the new job to v1. Unfortunately
I
> > > did not pay attention to how she then managed the numbering following my
> > > observation.
> > >
> > > What would be the best course of action at this point?
> >
> > My guess is that this patch gets ignored because it has a lower version
> > number than a previous patch.
> >
> > Take the feedback given here, and rev to
> > [PATCH v5] staging: axis-fifo: initialize timeouts in probe only
> >
> > Be sure the Changelog, below the --- explains the journey.
> >
> > Changes in v5:
> >
> > Changes in v4:
> >
> > Changes in v3:
>
> > Changes in v2:
> Hey Alison!
> Based on Nathan's feedback I am trying to recompile and send a patch
> without any warnings.
> As suggested by Fabio, I am running "make w=1 -jX" command to see if I
> get any warnings. But it is taking a lot of time, is there any way of
> speeding it up?
> If this doesn't work then I have to follow the steps to reproduce in lkp
> mail as you said before.
> After dealing with these warnings I will send a [PATCH v5], following
> your instructions above.
> Kindly, let me know if I am on the wrong track.
> Thank you!
>
> Regards,
> Khadija
>
> > > Fabio

Aside from what I said and asked for with the other message of this same
thread, please take note that you can build a specific module if you prefer
not to re-build the whole kernel and other modules at the same time.

I'm pretty sure that the instructions to do so are in the OutreachyFirstPatch
tutorial.

If they are not there, please let us know.

Fabio




2023-03-15 13:56:54

by Khadija Kamran

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Wed, Mar 15, 2023 at 02:13:51PM +0100, Fabio M. De Francesco wrote:
> On mercoled? 15 marzo 2023 13:32:55 CET Khadija Kamran wrote:
> > On Tue, Mar 14, 2023 at 04:57:47PM -0700, Alison Schofield wrote:
> > > My guess is that this patch gets ignored because it has a lower version
> > > number than a previous patch.
> > >
> > > Take the feedback given here, and rev to
> > > [PATCH v5] staging: axis-fifo: initialize timeouts in probe only
> > >
> > > Be sure the Changelog, below the --- explains the journey.
> > >
> > > Changes in v5:
> > >
> > > Changes in v4:
> > >
> > > Changes in v3:
> >
> > > Changes in v2:
> > Hey Alison!
>
> Hi Khadija,
>
> Please put one or two blank lines between the last message you are replying
> and the new you are writing (exactly as I'm doing here between "Hey Alison!"
> and "Hi Khadija").
>

Hey Fabio!

Sorry about that. This was pointed by Alison before and I have been
adding spaces since then. Hopefully I am doing it right this time.

> > Based on Nathan's feedback I am trying to recompile and send a patch
> > without any warnings.
>
> Great!
>
> > As suggested by Fabio, I am running "make w=1 -jX" command to see if I
> > get any warnings.
>
> I suppose that "w=1" is a typo. The option is enabled with "W=1" (capital
> case, Linux and all UNIX-like are case-sensitive).
>

Okay. I should re-run it with "W=1".

> > But it is taking a lot of time, is there any way of
> > speeding it up?
>
> What is you choice for 'X' in "-jX"?

I used "-j4".

> Did you try with the exact number of logical cores?
> Are you building into a VM with enough logical cores?
> If you are building into a VM, did you reserve enough RAM?

I am using Ubuntu 22.04.01 with the help of VM on VMware.
My machine has 13GB RAM and 2 processors(4 cores each).


> Please read carefully my questions above and try to understand your
> environment and reply, so that I can help you more effectively.
>
> > If this doesn't work then I have to follow the steps to reproduce in lkp
> > mail as you said before.
>
> The steps to reproduce will take your precious time and use more resources.
> Again, try to respond my questions.
>
> > After dealing with these warnings I will send a [PATCH v5], following
> > your instructions above.
>
> Sorry for inadvertently overlooking to warn you about to send a message to
> Greg and ask him to drop your first 3 + 1 patches. Now you are doing good by
> following what Alison suggested: send v5 and write the log of revisions under
> the three dashes (exactly how Alison explained).
>
> > Kindly, let me know if I am on the wrong track.
> > Thank you!
> >
>
> I think you are in the right track.
> Let's try to speed up your builds because you'll need to build again your
> kernel many, many times for future works.
>

Okay great! Thank you.

Regards,
Khadija

> Thanks,
>
> Fabio
>
>

2023-03-15 14:22:55

by Khadija Kamran

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Wed, Mar 15, 2023 at 02:34:31PM +0100, Fabio M. De Francesco wrote:
> Aside from what I said and asked for with the other message of this same
> thread, please take note that you can build a specific module if you prefer
> not to re-build the whole kernel and other modules at the same time.
>
> I'm pretty sure that the instructions to do so are in the OutreachyFirstPatch
> tutorial.
>
> If they are not there, please let us know.
>
> Fabio

Hey Fabio!

In the Outreachy FirstPatchTutorial under the 'Compiling only part of
the kernel' section there are ways to compile only some part of the
kernel.

I have tried using "make W=1 drivers/staging/axis-fifo/" and it says
'nothing to be done for'.

Should I start with the steps to reproduce? :'(

Thank you!

Regards,
Khadija


>
>
>

2023-03-15 15:07:11

by Nathan Chancellor

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

Hi Khadija,

On Wed, Mar 15, 2023 at 07:22:39PM +0500, Khadija Kamran wrote:
> On Wed, Mar 15, 2023 at 02:34:31PM +0100, Fabio M. De Francesco wrote:
> > Aside from what I said and asked for with the other message of this same
> > thread, please take note that you can build a specific module if you prefer
> > not to re-build the whole kernel and other modules at the same time.
> >
> > I'm pretty sure that the instructions to do so are in the OutreachyFirstPatch
> > tutorial.
> >
> > If they are not there, please let us know.
> >
> > Fabio
>
> Hey Fabio!
>
> In the Outreachy FirstPatchTutorial under the 'Compiling only part of
> the kernel' section there are ways to compile only some part of the
> kernel.
>
> I have tried using "make W=1 drivers/staging/axis-fifo/" and it says
> 'nothing to be done for'.

Is CONFIG_XIL_AXIS_FIFO enabled in your configuration?

> Should I start with the steps to reproduce? :'(

I did see a report of this same warning occurring with GCC but the
report I commented on cane from clang/LLVM; using that toolchain may
make it easier for you to reproduce this issue. The robot's reproduction
instructions are fine but I think doing it manually is actually simpler.

Ubuntu 22.04 should have a pretty modern version of clang/LLVM, which
you can install via 'sudo apt install clang lld llvm'.
https://apt.llvm.org is another resource.

First, we will grab the configuration that was provided in the report:

$ wget -O .config https://download.01.org/0day-ci/archive/20230314/[email protected]/config

Next, we want to make sure the configuration is synced, since we are
technically changing compilers:

$ make -j"$(nproc)" ARCH=arm64 LLVM=1 olddefconfig

Finally, you should be able to build that object file and see the
warning:

$ make -j"$(nproc)" ARCH=arm64 LLVM=1 drivers/staging/axis-fifo/
...
drivers/staging/axis-fifo/axis-fifo.c:817:18: warning: implicit conversion from 'long' to 'int' changes value from 9223372036854775807 to -1 [-Wconstant-conversion]
read_timeout = MAX_SCHEDULE_TIMEOUT;
~ ^~~~~~~~~~~~~~~~~~~~
./include/linux/sched.h:296:31: note: expanded from macro 'MAX_SCHEDULE_TIMEOUT'
#define MAX_SCHEDULE_TIMEOUT LONG_MAX
^~~~~~~~
./include/vdso/limits.h:11:19: note: expanded from macro 'LONG_MAX'
#define LONG_MAX ((long)(~0UL >> 1))
^~~~~~~~~~~~~~~~~
drivers/staging/axis-fifo/axis-fifo.c:822:19: warning: implicit conversion from 'long' to 'int' changes value from 9223372036854775807 to -1 [-Wconstant-conversion]
write_timeout = MAX_SCHEDULE_TIMEOUT;
~ ^~~~~~~~~~~~~~~~~~~~
./include/linux/sched.h:296:31: note: expanded from macro 'MAX_SCHEDULE_TIMEOUT'
#define MAX_SCHEDULE_TIMEOUT LONG_MAX
^~~~~~~~
./include/vdso/limits.h:11:19: note: expanded from macro 'LONG_MAX'
#define LONG_MAX ((long)(~0UL >> 1))
^~~~~~~~~~~~~~~~~
2 warnings generated.

Just repeat the last step as you investigate. If you have any further
issues or questions, please let me know. For the record, I am not
associated with Outreachy (I am one of the maintainers of clang/LLVM
support in the kernel), so if I have messed something up or overstepped
some boundary, I do apologize.

Cheers,
Nathan

2023-03-15 16:12:25

by Alison Schofield

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Wed, Mar 15, 2023 at 07:22:39PM +0500, Khadija Kamran wrote:
> On Wed, Mar 15, 2023 at 02:34:31PM +0100, Fabio M. De Francesco wrote:
> > Aside from what I said and asked for with the other message of this same
> > thread, please take note that you can build a specific module if you prefer
> > not to re-build the whole kernel and other modules at the same time.
> >
> > I'm pretty sure that the instructions to do so are in the OutreachyFirstPatch
> > tutorial.
> >
> > If they are not there, please let us know.
> >
> > Fabio
>
> Hey Fabio!
>
> In the Outreachy FirstPatchTutorial under the 'Compiling only part of
> the kernel' section there are ways to compile only some part of the
> kernel.
>
> I have tried using "make W=1 drivers/staging/axis-fifo/" and it says
> 'nothing to be done for'.
>
> Should I start with the steps to reproduce? :'(

Khadija,

I've applied your patch and it fails to compile with the warnings
that LKP reports.

If you are doing: $ make drivers/staging/axis-fifo/

and it is saying 'nothing to be done...'

You either have not changed anything since the last compile, or you
do not have the module configured.

I suspect it has never compiled for you and you need to look at
the direction in the tutorial for 'Recompiling the driver' section
and learn how to use make menuconfig.

This driver has a couple of dependencies described in the Kconfig
file. You will not even see the 'XIL_AXIS_FIFO' option until you
turn on 'OF' and HAS_'IOMEM'.

See the drivers/staging/axis-fifo/Kconfig

Please confirm that you have compiled the driver before chasing
after the build env that lkp reports.

Thanks,
Alison


>
> Thank you!
>
> Regards,
> Khadija
>
>
> >
> >
> >

2023-03-15 16:24:32

by Fabio M. De Francesco

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On mercoled? 15 marzo 2023 16:06:56 CET Nathan Chancellor wrote:
> Hi Khadija,
>
> On Wed, Mar 15, 2023 at 07:22:39PM +0500, Khadija Kamran wrote:
> > On Wed, Mar 15, 2023 at 02:34:31PM +0100, Fabio M. De Francesco wrote:
> > > Aside from what I said and asked for with the other message of this same
> > > thread, please take note that you can build a specific module if you
> > > prefer
> > > not to re-build the whole kernel and other modules at the same time.
> > >
> > > I'm pretty sure that the instructions to do so are in the
> > > OutreachyFirstPatch
> > > tutorial.
> > >
> > > If they are not there, please let us know.
> > >
> > > Fabio
> >
> > Hey Fabio!
> >
> > In the Outreachy FirstPatchTutorial under the 'Compiling only part of
> > the kernel' section there are ways to compile only some part of the
> > kernel.
> >
> > I have tried using "make W=1 drivers/staging/axis-fifo/" and it says
> > 'nothing to be done for'.
>
> Is CONFIG_XIL_AXIS_FIFO enabled in your configuration?

@Khadija:

Maybe that you won't find CONFIG_XIL_AXIS_FIFO among the staging drivers
entries while using "make menuconfig" (or the other similar commands for
working with the configuration).

In that case, first enable the CONFIG_OF option if disabled.
CONFIG_XIL_AXIS_FIFO depends on the former.

For "CONFIG_XIL_AXIS_FIFO depends on CONFIG_OF" we mean that you can the
former if and only if the latter is already enabled.

Actually, CONFIG_XIL_AXIS_FIFO depends also on two other options. However you
shouldn't worry about those others because they are commonly enabled in more
than 99% of PC distributions.

> > Should I start with the steps to reproduce? :'(
>
> I did see a report of this same warning occurring with GCC but the
> report I commented on cane from clang/LLVM; using that toolchain may
> make it easier for you to reproduce this issue. The robot's reproduction
> instructions are fine but I think doing it manually is actually simpler.
>
> Ubuntu 22.04 should have a pretty modern version of clang/LLVM, which
> you can install via 'sudo apt install clang lld llvm'.
> https://apt.llvm.org is another resource.

@Nathan: Congratulations, you now have another clang/LLVM adept!
(Obviously, I'm just kidding :-))

Fabio




2023-03-15 16:44:02

by Khadija Kamran

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Wed, Mar 15, 2023 at 09:09:16AM -0700, Alison Schofield wrote:
> On Wed, Mar 15, 2023 at 07:22:39PM +0500, Khadija Kamran wrote:
> > On Wed, Mar 15, 2023 at 02:34:31PM +0100, Fabio M. De Francesco wrote:
> > > Aside from what I said and asked for with the other message of this same
> > > thread, please take note that you can build a specific module if you prefer
> > > not to re-build the whole kernel and other modules at the same time.
> > >
> > > I'm pretty sure that the instructions to do so are in the OutreachyFirstPatch
> > > tutorial.
> > >
> > > If they are not there, please let us know.
> > >
> > > Fabio
> >
> > Hey Fabio!
> >
> > In the Outreachy FirstPatchTutorial under the 'Compiling only part of
> > the kernel' section there are ways to compile only some part of the
> > kernel.
> >
> > I have tried using "make W=1 drivers/staging/axis-fifo/" and it says
> > 'nothing to be done for'.
> >
> > Should I start with the steps to reproduce? :'(
>
> Khadija,
>
> I've applied your patch and it fails to compile with the warnings
> that LKP reports.
>
> If you are doing: $ make drivers/staging/axis-fifo/
>
> and it is saying 'nothing to be done...'
>
> You either have not changed anything since the last compile, or you
> do not have the module configured.


Hey Alison!
I might've written the statement wrong. Actually, here is the output of
make drivers/staging/axis-fifo

YACC scripts/genksyms/parse.tab.[ch]
HOSTCC scripts/genksyms/parse.tab.o
HOSTCC scripts/genksyms/lex.lex.o
HOSTLD scripts/genksyms/genksyms
CC scripts/mod/empty.o
MKELF scripts/mod/elfconfig.h
HOSTCC scripts/mod/modpost.o
CC scripts/mod/devicetable-offsets.s
HOSTCC scripts/mod/file2alias.o
HOSTCC scripts/mod/sumversion.o
HOSTLD scripts/mod/modpost
CC kernel/bounds.s
CC arch/x86/kernel/asm-offsets.s
CALL scripts/checksyscalls.sh
DESCEND objtool
INSTALL libsubcmd_headers
DESCEND bpf/resolve_btfids
INSTALL libsubcmd_headers
make[3]: Nothing to be done for 'drivers/staging/axis-fifo/'.


>
> I suspect it has never compiled for you and you need to look at
> the direction in the tutorial for 'Recompiling the driver' section
> and learn how to use make menuconfig.
>
> This driver has a couple of dependencies described in the Kconfig
> file. You will not even see the 'XIL_AXIS_FIFO' option until you
> turn on 'OF' and HAS_'IOMEM'.
>
> See the drivers/staging/axis-fifo/Kconfig
>
> Please confirm that you have compiled the driver before chasing
> after the build env that lkp reports.
>

Sorry, I made a mistake here.

I did not change the driver 'Xilinx AXI-Stream FIFO IP core driver' to
being compiled as a module by typing 'm'.
Is that the problem here?

Also, now when I try to change it by pressing 'm', it is not working.
And I have tried many times but I cannot change the driver from '*' to
'm'.

Thank you!
Regards,
Khadija


> Thanks,
> Alison
>
>
> >
> > Thank you!
> >
> > Regards,
> > Khadija
> >
> >
> > >
> > >
> > >

2023-03-15 16:46:17

by Fabio M. De Francesco

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On mercoled? 15 marzo 2023 14:56:27 CET Khadija Kamran wrote:
> On Wed, Mar 15, 2023 at 02:13:51PM +0100, Fabio M. De Francesco wrote:
> > On mercoled? 15 marzo 2023 13:32:55 CET Khadija Kamran wrote:
> > > On Tue, Mar 14, 2023 at 04:57:47PM -0700, Alison Schofield wrote:
> > > > My guess is that this patch gets ignored because it has a lower
version
> > > > number than a previous patch.
> > > >
> > > > Take the feedback given here, and rev to
> > > > [PATCH v5] staging: axis-fifo: initialize timeouts in probe only
> > > >
> > > > Be sure the Changelog, below the --- explains the journey.
> > > >
> > > > Changes in v5:
> > > >
> > > > Changes in v4:
> > > >
> > > > Changes in v3:
> > >
> > > > Changes in v2:
> > > Hey Alison!
> >
> > Hi Khadija,
> >
> > Please put one or two blank lines between the last message you are
replying
> > and the new you are writing (exactly as I'm doing here between "Hey
Alison!"
> > and "Hi Khadija").
>
> Hey Fabio!
>
> Sorry about that. This was pointed by Alison before and I have been
> adding spaces since then. Hopefully I am doing it right this time.
>
You are doing right this time :-)
>
> > > Based on Nathan's feedback I am trying to recompile and send a patch
> > > without any warnings.
> >
> > Great!
> >
> > > As suggested by Fabio, I am running "make w=1 -jX" command to see if I
> > > get any warnings.
> >
> > I suppose that "w=1" is a typo. The option is enabled with "W=1" (capital
> > case, Linux and all UNIX-like are case-sensitive).
>
> Okay. I should re-run it with "W=1".
>
> > > But it is taking a lot of time, is there any way of
> > > speeding it up?
> >
> > What is you choice for 'X' in "-jX"?
>
> I used "-j4".
>
> > Did you try with the exact number of logical cores?
> > Are you building into a VM with enough logical cores?
> > If you are building into a VM, did you reserve enough RAM?
>
> I am using Ubuntu 22.04.01 with the help of VM on VMware.
> My machine has 13GB RAM and 2 processors(4 cores each).

Therefore, you are using a Linux guest on a Linux host. This is a wise choice.
However, you didn't say where you are running your builds...

I mean, the better things to do are the following steps:

1) Your workspace with the staging tree should stay in the host.
2) Shut down your guest in order to have all RAM and all logical processors
available for the build.
3) Run "make -j8" in the host. Since you shutdown your guest VM you can use
all 8 logical cores and the maximum available RAM (without the VM draining
resources while building)
4) When the build is done, switch on your VM on VMware with at least 4 logical
cores and 6GB of reserved RAM.
5) Mount your cloned base directory as a shared folder between host and guest.
6) In the guest, 'cd' to the shared folder and then run "make modules_install
install" (in the guest, attention). This will install and configure the
kernel, the modules, GRUB2 and everything else in your guest VM.
7) Reboot the VM and test your patches.

This procedure will speed up your next builds.
The fundamental point is that you don't need to partition precious resources
while building, Do everything without running the VM and switch it on only for
install and tests. Since you only build in the host but never install and boot
in it, you don't risk any system's damage.

This is what I do for Kernel development purposes.

I hope it helps to answer your question about how to run fast recompilation.

Fabio


>
> > Please read carefully my questions above and try to understand your
> > environment and reply, so that I can help you more effectively.
> >
> > > If this doesn't work then I have to follow the steps to reproduce in lkp
> > > mail as you said before.
> >
> > The steps to reproduce will take your precious time and use more
resources.
> > Again, try to respond my questions.
> >
> > > After dealing with these warnings I will send a [PATCH v5], following
> > > your instructions above.
> >
> > Sorry for inadvertently overlooking to warn you about to send a message to
> > Greg and ask him to drop your first 3 + 1 patches. Now you are doing good
by
> > following what Alison suggested: send v5 and write the log of revisions
> > under
> > the three dashes (exactly how Alison explained).
> >
> > > Kindly, let me know if I am on the wrong track.
> > > Thank you!
> >
> > I think you are in the right track.
> > Let's try to speed up your builds because you'll need to build again your
> > kernel many, many times for future works.
>
> Okay great! Thank you.
>
> Regards,
> Khadija
>
> > Thanks,
> >
> > Fabio





2023-03-16 07:40:45

by Julia Lawall

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

> Just repeat the last step as you investigate. If you have any further
> issues or questions, please let me know. For the record, I am not
> associated with Outreachy (I am one of the maintainers of clang/LLVM
> support in the kernel), so if I have messed something up or overstepped
> some boundary, I do apologize.

Definitely not. Thanks for helping out!

julia

2023-03-16 09:51:02

by Khadija Kamran

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Wed, Mar 15, 2023 at 05:44:47PM +0100, Fabio M. De Francesco wrote:
> On mercoled? 15 marzo 2023 14:56:27 CET Khadija Kamran wrote:
> > On Wed, Mar 15, 2023 at 02:13:51PM +0100, Fabio M. De Francesco wrote:
> > > On mercoled? 15 marzo 2023 13:32:55 CET Khadija Kamran wrote:
> > > > On Tue, Mar 14, 2023 at 04:57:47PM -0700, Alison Schofield wrote:
> > > > > My guess is that this patch gets ignored because it has a lower
> version
> > > > > number than a previous patch.
> > > > >
> > > > > Take the feedback given here, and rev to
> > > > > [PATCH v5] staging: axis-fifo: initialize timeouts in probe only
> > > > >
> > > > > Be sure the Changelog, below the --- explains the journey.
> > > > >
> > > > > Changes in v5:
> > > > >
> > > > > Changes in v4:
> > > > >
> > > > > Changes in v3:
> > > >
> > > > > Changes in v2:
> > > > Hey Alison!
> > >
> > > Hi Khadija,
> > >
> > > Please put one or two blank lines between the last message you are
> replying
> > > and the new you are writing (exactly as I'm doing here between "Hey
> Alison!"
> > > and "Hi Khadija").
> >
> > Hey Fabio!
> >
> > Sorry about that. This was pointed by Alison before and I have been
> > adding spaces since then. Hopefully I am doing it right this time.
> >
> You are doing right this time :-)
> >
> > > > Based on Nathan's feedback I am trying to recompile and send a patch
> > > > without any warnings.
> > >
> > > Great!
> > >
> > > > As suggested by Fabio, I am running "make w=1 -jX" command to see if I
> > > > get any warnings.
> > >
> > > I suppose that "w=1" is a typo. The option is enabled with "W=1" (capital
> > > case, Linux and all UNIX-like are case-sensitive).
> >
> > Okay. I should re-run it with "W=1".
> >
> > > > But it is taking a lot of time, is there any way of
> > > > speeding it up?
> > >
> > > What is you choice for 'X' in "-jX"?
> >
> > I used "-j4".
> >
> > > Did you try with the exact number of logical cores?
> > > Are you building into a VM with enough logical cores?
> > > If you are building into a VM, did you reserve enough RAM?
> >
> > I am using Ubuntu 22.04.01 with the help of VM on VMware.
> > My machine has 13GB RAM and 2 processors(4 cores each).
>
> Therefore, you are using a Linux guest on a Linux host. This is a wise choice.
> However, you didn't say where you are running your builds...

Hey Fabio!

I am not using a Linux guest on Linux host. Sorry if I did not explain
it right. I am using Windows 10 and in order to run Ubuntu, I have
created a VM(on VMWare). This VM has 13GB RAM and 2 processors(4 cores
each).
Thank you!

Regards,
Khadija
>
> I mean, the better things to do are the following steps:
>
> 1) Your workspace with the staging tree should stay in the host.
> 2) Shut down your guest in order to have all RAM and all logical processors
> available for the build.
> 3) Run "make -j8" in the host. Since you shutdown your guest VM you can use
> all 8 logical cores and the maximum available RAM (without the VM draining
> resources while building)
> 4) When the build is done, switch on your VM on VMware with at least 4 logical
> cores and 6GB of reserved RAM.
> 5) Mount your cloned base directory as a shared folder between host and guest.
> 6) In the guest, 'cd' to the shared folder and then run "make modules_install
> install" (in the guest, attention). This will install and configure the
> kernel, the modules, GRUB2 and everything else in your guest VM.
> 7) Reboot the VM and test your patches.
>
> This procedure will speed up your next builds.
> The fundamental point is that you don't need to partition precious resources
> while building, Do everything without running the VM and switch it on only for
> install and tests. Since you only build in the host but never install and boot
> in it, you don't risk any system's damage.
>
> This is what I do for Kernel development purposes.
>
> I hope it helps to answer your question about how to run fast recompilation.
>
> Fabio
>
>
> >
> > > Please read carefully my questions above and try to understand your
> > > environment and reply, so that I can help you more effectively.
> > >
> > > > If this doesn't work then I have to follow the steps to reproduce in lkp
> > > > mail as you said before.
> > >
> > > The steps to reproduce will take your precious time and use more
> resources.
> > > Again, try to respond my questions.
> > >
> > > > After dealing with these warnings I will send a [PATCH v5], following
> > > > your instructions above.
> > >
> > > Sorry for inadvertently overlooking to warn you about to send a message to
> > > Greg and ask him to drop your first 3 + 1 patches. Now you are doing good
> by
> > > following what Alison suggested: send v5 and write the log of revisions
> > > under
> > > the three dashes (exactly how Alison explained).
> > >
> > > > Kindly, let me know if I am on the wrong track.
> > > > Thank you!
> > >
> > > I think you are in the right track.
> > > Let's try to speed up your builds because you'll need to build again your
> > > kernel many, many times for future works.
> >
> > Okay great! Thank you.
> >
> > Regards,
> > Khadija
> >
> > > Thanks,
> > >
> > > Fabio
>
>
>
>

2023-03-16 10:17:17

by Khadija Kamran

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Wed, Mar 15, 2023 at 05:24:23PM +0100, Fabio M. De Francesco wrote:
> On mercoled? 15 marzo 2023 16:06:56 CET Nathan Chancellor wrote:
> > Hi Khadija,
> >
> > On Wed, Mar 15, 2023 at 07:22:39PM +0500, Khadija Kamran wrote:
> > > On Wed, Mar 15, 2023 at 02:34:31PM +0100, Fabio M. De Francesco wrote:
> > > > Aside from what I said and asked for with the other message of this same
> > > > thread, please take note that you can build a specific module if you
> > > > prefer
> > > > not to re-build the whole kernel and other modules at the same time.
> > > >
> > > > I'm pretty sure that the instructions to do so are in the
> > > > OutreachyFirstPatch
> > > > tutorial.
> > > >
> > > > If they are not there, please let us know.
> > > >
> > > > Fabio
> > >
> > > Hey Fabio!
> > >
> > > In the Outreachy FirstPatchTutorial under the 'Compiling only part of
> > > the kernel' section there are ways to compile only some part of the
> > > kernel.
> > >
> > > I have tried using "make W=1 drivers/staging/axis-fifo/" and it says
> > > 'nothing to be done for'.
> >
> > Is CONFIG_XIL_AXIS_FIFO enabled in your configuration?
>
> @Khadija:
>
> Maybe that you won't find CONFIG_XIL_AXIS_FIFO among the staging drivers
> entries while using "make menuconfig" (or the other similar commands for
> working with the configuration).
>
> In that case, first enable the CONFIG_OF option if disabled.
> CONFIG_XIL_AXIS_FIFO depends on the former.
>
> For "CONFIG_XIL_AXIS_FIFO depends on CONFIG_OF" we mean that you can the
> former if and only if the latter is already enabled.
>
> Actually, CONFIG_XIL_AXIS_FIFO depends also on two other options. However you
> shouldn't worry about those others because they are commonly enabled in more
> than 99% of PC distributions.
>

Hey Fabio!

I have checked in my .config file and both options are enabled. The file
says,
CONFIG_OF=y
CONFIG_HAS_IOMEM=y

The "make menuconfig" shows 'Xilinx AXI-Stream FIFO IP core driver' with
built-in selection [*].
The problem I am having here is that in the OutreachyFIrstPatch
tutorial, it asks you to change [*] -> [m], but I cannot change it.

Kindly help me with this!

Thank you!
Regards,
Khadija



> > > Should I start with the steps to reproduce? :'(
> >
> > I did see a report of this same warning occurring with GCC but the
> > report I commented on cane from clang/LLVM; using that toolchain may
> > make it easier for you to reproduce this issue. The robot's reproduction
> > instructions are fine but I think doing it manually is actually simpler.
> >
> > Ubuntu 22.04 should have a pretty modern version of clang/LLVM, which
> > you can install via 'sudo apt install clang lld llvm'.
> > https://apt.llvm.org is another resource.
>
> @Nathan: Congratulations, you now have another clang/LLVM adept!
> (Obviously, I'm just kidding :-))
>
> Fabio
>
>
>

2023-03-16 10:36:29

by Khadija Kamran

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Wed, Mar 15, 2023 at 09:09:16AM -0700, Alison Schofield wrote:
> On Wed, Mar 15, 2023 at 07:22:39PM +0500, Khadija Kamran wrote:
> > On Wed, Mar 15, 2023 at 02:34:31PM +0100, Fabio M. De Francesco wrote:
> > > Aside from what I said and asked for with the other message of this same
> > > thread, please take note that you can build a specific module if you prefer
> > > not to re-build the whole kernel and other modules at the same time.
> > >
> > > I'm pretty sure that the instructions to do so are in the OutreachyFirstPatch
> > > tutorial.
> > >
> > > If they are not there, please let us know.
> > >
> > > Fabio
> >
> > Hey Fabio!
> >
> > In the Outreachy FirstPatchTutorial under the 'Compiling only part of
> > the kernel' section there are ways to compile only some part of the
> > kernel.
> >
> > I have tried using "make W=1 drivers/staging/axis-fifo/" and it says
> > 'nothing to be done for'.
> >
> > Should I start with the steps to reproduce? :'(
>
> Khadija,
>
> I've applied your patch and it fails to compile with the warnings
> that LKP reports.
>
> If you are doing: $ make drivers/staging/axis-fifo/
>
> and it is saying 'nothing to be done...'
>
> You either have not changed anything since the last compile, or you
> do not have the module configured.
>

Hey Alison!
I might've written the statement wrong. Actually, here is the output of
make drivers/staging/axis-fifo

YACC scripts/genksyms/parse.tab.[ch]
HOSTCC scripts/genksyms/parse.tab.o
HOSTCC scripts/genksyms/lex.lex.o
HOSTLD scripts/genksyms/genksyms
CC scripts/mod/empty.o
MKELF scripts/mod/elfconfig.h
HOSTCC scripts/mod/modpost.o
CC scripts/mod/devicetable-offsets.s
HOSTCC scripts/mod/file2alias.o
HOSTCC scripts/mod/sumversion.o
HOSTLD scripts/mod/modpost
CC kernel/bounds.s
CC arch/x86/kernel/asm-offsets.s
CALL scripts/checksyscalls.sh
DESCEND objtool
INSTALL libsubcmd_headers
DESCEND bpf/resolve_btfids
INSTALL libsubcmd_headers
make[3]: Nothing to be done for 'drivers/staging/axis-fifo/'.


> I suspect it has never compiled for you and you need to look at
> the direction in the tutorial for 'Recompiling the driver' section
> and learn how to use make menuconfig.
>
> This driver has a couple of dependencies described in the Kconfig
> file. You will not even see the 'XIL_AXIS_FIFO' option until you
> turn on 'OF' and HAS_'IOMEM'.
>
> See the drivers/staging/axis-fifo/Kconfig
>
> Please confirm that you have compiled the driver before chasing
> after the build env that lkp reports.
>
> Thanks,
> Alison

Sorry, I made a mistake here.

I did not change the driver 'Xilinx AXI-Stream FIFO IP core driver' to
being compiled as a module by typing 'm'.
Is that the problem here?

Also, now when I try to change it by pressing 'm', it is not working.
And I have tried many times but I cannot change the driver from '*' to
'm'.

Kindly help me with this.

Regards,
Khadija



2023-03-16 10:47:17

by Khadija Kamran

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Wed, Mar 15, 2023 at 08:06:56AM -0700, Nathan Chancellor wrote:
> Hi Khadija,
>
> On Wed, Mar 15, 2023 at 07:22:39PM +0500, Khadija Kamran wrote:
> > On Wed, Mar 15, 2023 at 02:34:31PM +0100, Fabio M. De Francesco wrote:
> > > Aside from what I said and asked for with the other message of this same
> > > thread, please take note that you can build a specific module if you prefer
> > > not to re-build the whole kernel and other modules at the same time.
> > >
> > > I'm pretty sure that the instructions to do so are in the OutreachyFirstPatch
> > > tutorial.
> > >
> > > If they are not there, please let us know.
> > >
> > > Fabio
> >
> > Hey Fabio!
> >
> > In the Outreachy FirstPatchTutorial under the 'Compiling only part of
> > the kernel' section there are ways to compile only some part of the
> > kernel.
> >
> > I have tried using "make W=1 drivers/staging/axis-fifo/" and it says
> > 'nothing to be done for'.
>
> Is CONFIG_XIL_AXIS_FIFO enabled in your configuration?
>

Hey Nathan!
Thank you for all the help.
'Xilinx AXI-Stream FIFO IP core driver' option in my menufig shows [*]
and I cannot change it to ['m'] following all the intrustcions in
OutreachyFIrstPatch step by step.

> > Should I start with the steps to reproduce? :'(
>
> I did see a report of this same warning occurring with GCC but the
> report I commented on cane from clang/LLVM; using that toolchain may
> make it easier for you to reproduce this issue. The robot's reproduction
> instructions are fine but I think doing it manually is actually simpler.
>
> Ubuntu 22.04 should have a pretty modern version of clang/LLVM, which
> you can install via 'sudo apt install clang lld llvm'.
> https://apt.llvm.org is another resource.
>
> First, we will grab the configuration that was provided in the report:
>
> $ wget -O .config https://download.01.org/0day-ci/archive/20230314/[email protected]/config
>
> Next, we want to make sure the configuration is synced, since we are
> technically changing compilers:
>
> $ make -j"$(nproc)" ARCH=arm64 LLVM=1 olddefconfig
>
> Finally, you should be able to build that object file and see the
> warning:
>
> $ make -j"$(nproc)" ARCH=arm64 LLVM=1 drivers/staging/axis-fifo/
>

It did not show any warnings. A followed all your instructions step by
step.

Thanks again!

Regards,
Khadija

> drivers/staging/axis-fifo/axis-fifo.c:817:18: warning: implicit conversion from 'long' to 'int' changes value from 9223372036854775807 to -1 [-Wconstant-conversion]
> read_timeout = MAX_SCHEDULE_TIMEOUT;
> ~ ^~~~~~~~~~~~~~~~~~~~
> ./include/linux/sched.h:296:31: note: expanded from macro 'MAX_SCHEDULE_TIMEOUT'
> #define MAX_SCHEDULE_TIMEOUT LONG_MAX
> ^~~~~~~~
> ./include/vdso/limits.h:11:19: note: expanded from macro 'LONG_MAX'
> #define LONG_MAX ((long)(~0UL >> 1))
> ^~~~~~~~~~~~~~~~~
> drivers/staging/axis-fifo/axis-fifo.c:822:19: warning: implicit conversion from 'long' to 'int' changes value from 9223372036854775807 to -1 [-Wconstant-conversion]
> write_timeout = MAX_SCHEDULE_TIMEOUT;
> ~ ^~~~~~~~~~~~~~~~~~~~
> ./include/linux/sched.h:296:31: note: expanded from macro 'MAX_SCHEDULE_TIMEOUT'
> #define MAX_SCHEDULE_TIMEOUT LONG_MAX
> ^~~~~~~~
> ./include/vdso/limits.h:11:19: note: expanded from macro 'LONG_MAX'
> #define LONG_MAX ((long)(~0UL >> 1))
> ^~~~~~~~~~~~~~~~~
> 2 warnings generated.
>
> Just repeat the last step as you investigate. If you have any further
> issues or questions, please let me know. For the record, I am not
> associated with Outreachy (I am one of the maintainers of clang/LLVM
> support in the kernel), so if I have messed something up or overstepped
> some boundary, I do apologize.
>
> Cheers,
> Nathan

2023-03-16 10:52:21

by Julia Lawall

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only



On Thu, 16 Mar 2023, Khadija Kamran wrote:

> On Wed, Mar 15, 2023 at 09:09:16AM -0700, Alison Schofield wrote:
> > On Wed, Mar 15, 2023 at 07:22:39PM +0500, Khadija Kamran wrote:
> > > On Wed, Mar 15, 2023 at 02:34:31PM +0100, Fabio M. De Francesco wrote:
> > > > Aside from what I said and asked for with the other message of this same
> > > > thread, please take note that you can build a specific module if you prefer
> > > > not to re-build the whole kernel and other modules at the same time.
> > > >
> > > > I'm pretty sure that the instructions to do so are in the OutreachyFirstPatch
> > > > tutorial.
> > > >
> > > > If they are not there, please let us know.
> > > >
> > > > Fabio
> > >
> > > Hey Fabio!
> > >
> > > In the Outreachy FirstPatchTutorial under the 'Compiling only part of
> > > the kernel' section there are ways to compile only some part of the
> > > kernel.
> > >
> > > I have tried using "make W=1 drivers/staging/axis-fifo/" and it says
> > > 'nothing to be done for'.
> > >
> > > Should I start with the steps to reproduce? :'(
> >
> > Khadija,
> >
> > I've applied your patch and it fails to compile with the warnings
> > that LKP reports.
> >
> > If you are doing: $ make drivers/staging/axis-fifo/
> >
> > and it is saying 'nothing to be done...'
> >
> > You either have not changed anything since the last compile, or you
> > do not have the module configured.
> >
>
> Hey Alison!
> I might've written the statement wrong. Actually, here is the output of
> make drivers/staging/axis-fifo
>
> YACC scripts/genksyms/parse.tab.[ch]
> HOSTCC scripts/genksyms/parse.tab.o
> HOSTCC scripts/genksyms/lex.lex.o
> HOSTLD scripts/genksyms/genksyms
> CC scripts/mod/empty.o
> MKELF scripts/mod/elfconfig.h
> HOSTCC scripts/mod/modpost.o
> CC scripts/mod/devicetable-offsets.s
> HOSTCC scripts/mod/file2alias.o
> HOSTCC scripts/mod/sumversion.o
> HOSTLD scripts/mod/modpost
> CC kernel/bounds.s
> CC arch/x86/kernel/asm-offsets.s
> CALL scripts/checksyscalls.sh
> DESCEND objtool
> INSTALL libsubcmd_headers
> DESCEND bpf/resolve_btfids
> INSTALL libsubcmd_headers
> make[3]: Nothing to be done for 'drivers/staging/axis-fifo/'.

Did you try make allyesconfig and make drivers/staging/axis-fifo/axis-fifo.o

I get no warnings with W=1, but I do get a .o file. I also tried

make drivers/staging/axis-fifo/axis-fifo.i

which just expands macros and simplifies ifdefs, and I see the various
code from the file. So I don't have the impression that it is getting
ifdef'd away in some obscure way. Perhaps I am missing something. Or
perhaps the warning in question only comres from LLVM?

julia

>
>
> > I suspect it has never compiled for you and you need to look at
> > the direction in the tutorial for 'Recompiling the driver' section
> > and learn how to use make menuconfig.
> >
> > This driver has a couple of dependencies described in the Kconfig
> > file. You will not even see the 'XIL_AXIS_FIFO' option until you
> > turn on 'OF' and HAS_'IOMEM'.
> >
> > See the drivers/staging/axis-fifo/Kconfig
> >
> > Please confirm that you have compiled the driver before chasing
> > after the build env that lkp reports.
> >
> > Thanks,
> > Alison
>
> Sorry, I made a mistake here.
>
> I did not change the driver 'Xilinx AXI-Stream FIFO IP core driver' to
> being compiled as a module by typing 'm'.
> Is that the problem here?
>
> Also, now when I try to change it by pressing 'm', it is not working.
> And I have tried many times but I cannot change the driver from '*' to
> 'm'.
>
> Kindly help me with this.
>
> Regards,
> Khadija
>
>
>
>

2023-03-16 11:13:32

by Fabio M. De Francesco

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On gioved? 16 marzo 2023 10:50:50 CET Khadija Kamran wrote:
> On Wed, Mar 15, 2023 at 05:44:47PM +0100, Fabio M. De Francesco wrote:
> > On mercoled? 15 marzo 2023 14:56:27 CET Khadija Kamran wrote:
> > > On Wed, Mar 15, 2023 at 02:13:51PM +0100, Fabio M. De Francesco wrote:
> > > > On mercoled? 15 marzo 2023 13:32:55 CET Khadija Kamran wrote:

[snip]

> > > > I suppose that "w=1" is a typo. The option is enabled with "W=1"
> > > > (capital
> > > > case, Linux and all UNIX-like are case-sensitive).
> > >
> > > Okay. I should re-run it with "W=1".
> > >
> > > > > But it is taking a lot of time, is there any way of
> > > > > speeding it up?
> > > >
> > > > What is you choice for 'X' in "-jX"?
> > >
> > > I used "-j4".
> > >
> > > > Did you try with the exact number of logical cores?
> > > > Are you building into a VM with enough logical cores?
> > > > If you are building into a VM, did you reserve enough RAM?
> > >
> > > I am using Ubuntu 22.04.01 with the help of VM on VMware.
> > > My machine has 13GB RAM and 2 processors(4 cores each).
> >
> > Therefore, you are using a Linux guest on a Linux host. This is a wise
> > choice. However, you didn't say where you are running your builds...
>
> Hey Fabio!
>
> I am not using a Linux guest on Linux host. Sorry if I did not explain
> it right. I am using Windows 10 and in order to run Ubuntu, I have
> created a VM(on VMWare). This VM has 13GB RAM and 2 processors(4 cores
> each).
> Thank you!
>
> Regards,
> Khadija

Khadija,

I'm not yet sure whether or not you are doing well with reserving 13GB to a
VM. First of all you should better use multiples of 2GB. Furthermore it looks
too much memory unless you have a total "real" RAM in your host exceeding
18GB. You must be sure you are leaving enough room for the Windows host to run
smoothly and avoid too much swapping to/from disk.

At the same time I doubt that you can reserve 8 logical cores (2x4) for
running the VM on VMWare.

However, we have time to investigate and adjust your configuration (if it
really needs to be fine tuned). I'll try to reach you on IRC, open a private
window, and assist with this task ASAP.

At the moment I'd prefer to see you focusing on getting your first patch
accepted by Greg.

Thanks,

Fabio

> > I mean, the better things to do are the following steps:
> >
> > 1) Your workspace with the staging tree should stay in the host.
> > 2) Shut down your guest in order to have all RAM and all logical
processors
> > available for the build.
> > 3) Run "make -j8" in the host. Since you shutdown your guest VM you can
use
> > all 8 logical cores and the maximum available RAM (without the VM draining
> > resources while building)
> > 4) When the build is done, switch on your VM on VMware with at least 4
> > logical cores and 6GB of reserved RAM.
> > 5) Mount your cloned base directory as a shared folder between host and
> > guest. 6) In the guest, 'cd' to the shared folder and then run "make
> > modules_install install" (in the guest, attention). This will install and
> > configure the kernel, the modules, GRUB2 and everything else in your guest
> > VM.
> > 7) Reboot the VM and test your patches.
> >
> > This procedure will speed up your next builds.
> > The fundamental point is that you don't need to partition precious
resources
> > while building, Do everything without running the VM and switch it on only
> > for install and tests. Since you only build in the host but never install
> > and boot in it, you don't risk any system's damage.
> >
> > This is what I do for Kernel development purposes.
> >
> > I hope it helps to answer your question about how to run fast
recompilation.
> >
> > Fabio
> >




2023-03-16 11:40:31

by Fabio M. De Francesco

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On gioved? 16 marzo 2023 11:17:05 CET Khadija Kamran wrote:
> On Wed, Mar 15, 2023 at 05:24:23PM +0100, Fabio M. De Francesco wrote:
> > On mercoled? 15 marzo 2023 16:06:56 CET Nathan Chancellor wrote:
> > > Hi Khadija,
> > >
> > > On Wed, Mar 15, 2023 at 07:22:39PM +0500, Khadija Kamran wrote:
> > > > On Wed, Mar 15, 2023 at 02:34:31PM +0100, Fabio M. De Francesco wrote:
> > > > > Aside from what I said and asked for with the other message of this
> > > > > same
> > > > > thread, please take note that you can build a specific module if you
> > > > > prefer
> > > > > not to re-build the whole kernel and other modules at the same time.
> > > > >
> > > > > I'm pretty sure that the instructions to do so are in the
> > > > > OutreachyFirstPatch
> > > > > tutorial.
> > > > >
> > > > > If they are not there, please let us know.
> > > > >
> > > > > Fabio
> > > >
> > > > Hey Fabio!
> > > >
> > > > In the Outreachy FirstPatchTutorial under the 'Compiling only part of
> > > > the kernel' section there are ways to compile only some part of the
> > > > kernel.
> > > >
> > > > I have tried using "make W=1 drivers/staging/axis-fifo/" and it says
> > > > 'nothing to be done for'.
> > >
> > > Is CONFIG_XIL_AXIS_FIFO enabled in your configuration?
> >
> > @Khadija:
> >
> > Maybe that you won't find CONFIG_XIL_AXIS_FIFO among the staging drivers
> > entries while using "make menuconfig" (or the other similar commands for
> > working with the configuration).
> >
> > In that case, first enable the CONFIG_OF option if disabled.
> > CONFIG_XIL_AXIS_FIFO depends on the former.
> >
> > For "CONFIG_XIL_AXIS_FIFO depends on CONFIG_OF" we mean that you can the
> > former if and only if the latter is already enabled.
> >
> > Actually, CONFIG_XIL_AXIS_FIFO depends also on two other options. However
> > you
> > shouldn't worry about those others because they are commonly enabled in
more
> > than 99% of PC distributions.
>
> Hey Fabio!
>
> I have checked in my .config file and both options are enabled. The file
> says,
> CONFIG_OF=y
> CONFIG_HAS_IOMEM=y
>
> The "make menuconfig" shows 'Xilinx AXI-Stream FIFO IP core driver' with
> built-in selection [*].
> The problem I am having here is that in the OutreachyFirstPatch
> tutorial, it asks you to change [*] -> [m], but I cannot change it.
>
> Kindly help me with this!

Hi Khadija,

Sorry for being here so late.

I just saw also your other messages with further help requests.
Please do other work while waiting for help on a specific issue.

If I understand you correctly, you are saying that you can only build axis-
fifo in-kernel because you cannot change the '*' to 'm'. Correct?

If so, you have probably touched other configuration options in a way that
forces that driver to work only if compiled in-kernel). Therefore, I'd restart
from scratch.

Please delete your current .config and make a copy of the one from your
running kernel. Then check the three options Alison and I were talking about.

Remember that you cannot see the axis-fifo module entry (CONFIG_XIL_AXIS_FIFO)
in "Device Drivers" -> "Staging Drivers" section if you have not yet enabled
(either with 'm' or 'y') CONFIG_OF and CONFIG_HAS_IOMEM.

Now you should end up with a visible CONFIG_XIL_AXIS_FIFO that can be enabled
as a module with 'm'.

Let me know if this procedure works.

Fabio

> Thank you!
> Regards,
> Khadija
>
> > > > Should I start with the steps to reproduce? :'(
> > >
> > > I did see a report of this same warning occurring with GCC but the
> > > report I commented on cane from clang/LLVM; using that toolchain may
> > > make it easier for you to reproduce this issue. The robot's reproduction
> > > instructions are fine but I think doing it manually is actually simpler.
> > >
> > > Ubuntu 22.04 should have a pretty modern version of clang/LLVM, which
> > > you can install via 'sudo apt install clang lld llvm'.
> > > https://apt.llvm.org is another resource.
> >
> > @Nathan: Congratulations, you now have another clang/LLVM adept!
> > (Obviously, I'm just kidding :-))
> >
> > Fabio





2023-03-16 11:42:13

by Khadija Kamran

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Wed, Mar 15, 2023 at 08:06:56AM -0700, Nathan Chancellor wrote:
> Hi Khadija,
>
> On Wed, Mar 15, 2023 at 07:22:39PM +0500, Khadija Kamran wrote:
> > On Wed, Mar 15, 2023 at 02:34:31PM +0100, Fabio M. De Francesco wrote:
> > > Aside from what I said and asked for with the other message of this same
> > > thread, please take note that you can build a specific module if you prefer
> > > not to re-build the whole kernel and other modules at the same time.
> > >
> > > I'm pretty sure that the instructions to do so are in the OutreachyFirstPatch
> > > tutorial.
> > >
> > > If they are not there, please let us know.
> > >
> > > Fabio
> >
> > Hey Fabio!
> >
> > In the Outreachy FirstPatchTutorial under the 'Compiling only part of
> > the kernel' section there are ways to compile only some part of the
> > kernel.
> >
> > I have tried using "make W=1 drivers/staging/axis-fifo/" and it says
> > 'nothing to be done for'.
>
> Is CONFIG_XIL_AXIS_FIFO enabled in your configuration?
>
> > Should I start with the steps to reproduce? :'(
>
> I did see a report of this same warning occurring with GCC but the
> report I commented on cane from clang/LLVM; using that toolchain may
> make it easier for you to reproduce this issue. The robot's reproduction
> instructions are fine but I think doing it manually is actually simpler.
>
> Ubuntu 22.04 should have a pretty modern version of clang/LLVM, which
> you can install via 'sudo apt install clang lld llvm'.
> https://apt.llvm.org is another resource.
>
> First, we will grab the configuration that was provided in the report:
>
> $ wget -O .config https://download.01.org/0day-ci/archive/20230314/[email protected]/config
>
> Next, we want to make sure the configuration is synced, since we are
> technically changing compilers:
>
> $ make -j"$(nproc)" ARCH=arm64 LLVM=1 olddefconfig
>
> Finally, you should be able to build that object file and see the
> warning:
>
> $ make -j"$(nproc)" ARCH=arm64 LLVM=1 drivers/staging/axis-fifo/
> ...
> drivers/staging/axis-fifo/axis-fifo.c:817:18: warning: implicit conversion from 'long' to 'int' changes value from 9223372036854775807 to -1 [-Wconstant-conversion]
> read_timeout = MAX_SCHEDULE_TIMEOUT;
> ~ ^~~~~~~~~~~~~~~~~~~~
> ./include/linux/sched.h:296:31: note: expanded from macro 'MAX_SCHEDULE_TIMEOUT'
> #define MAX_SCHEDULE_TIMEOUT LONG_MAX
> ^~~~~~~~
> ./include/vdso/limits.h:11:19: note: expanded from macro 'LONG_MAX'
> #define LONG_MAX ((long)(~0UL >> 1))
> ^~~~~~~~~~~~~~~~~
> drivers/staging/axis-fifo/axis-fifo.c:822:19: warning: implicit conversion from 'long' to 'int' changes value from 9223372036854775807 to -1 [-Wconstant-conversion]
> write_timeout = MAX_SCHEDULE_TIMEOUT;
> ~ ^~~~~~~~~~~~~~~~~~~~
> ./include/linux/sched.h:296:31: note: expanded from macro 'MAX_SCHEDULE_TIMEOUT'
> #define MAX_SCHEDULE_TIMEOUT LONG_MAX
> ^~~~~~~~
> ./include/vdso/limits.h:11:19: note: expanded from macro 'LONG_MAX'
> #define LONG_MAX ((long)(~0UL >> 1))
> ^~~~~~~~~~~~~~~~~
> 2 warnings generated.
>
> Just repeat the last step as you investigate. If you have any further
> issues or questions, please let me know. For the record, I am not
> associated with Outreachy (I am one of the maintainers of clang/LLVM
> support in the kernel), so if I have messed something up or overstepped
> some boundary, I do apologize.
>
> Cheers,
> Nathan

Hi Nathan!

Sorry about the last email where I said that this is not working. I was
working in the wrong branch, my bad.

This works! And it has helped me remove the warnings too. I have
replaced int datatype with long and the two warnings that were showing
are no longer there.

I am working on [PATCH v5] and I will submit it in no time.

This was great help. :)

Thank you!
Regards,
Khadija



2023-03-16 11:55:31

by Khadija Kamran

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Thu, Mar 16, 2023 at 12:39:31PM +0100, Fabio M. De Francesco wrote:
> Hi Khadija,
>
> Sorry for being here so late.
>
> I just saw also your other messages with further help requests.
> Please do other work while waiting for help on a specific issue.
>
> If I understand you correctly, you are saying that you can only build axis-
> fifo in-kernel because you cannot change the '*' to 'm'. Correct?
>

Yes, corect.

> If so, you have probably touched other configuration options in a way that
> forces that driver to work only if compiled in-kernel). Therefore, I'd restart
> from scratch.
>
> Please delete your current .config and make a copy of the one from your
> running kernel. Then check the three options Alison and I were talking about.
>
> Remember that you cannot see the axis-fifo module entry (CONFIG_XIL_AXIS_FIFO)
> in "Device Drivers" -> "Staging Drivers" section if you have not yet enabled
> (either with 'm' or 'y') CONFIG_OF and CONFIG_HAS_IOMEM.
>
> Now you should end up with a visible CONFIG_XIL_AXIS_FIFO that can be enabled
> as a module with 'm'.
>

Hey Fabio!

I am on it. I have undertood the steps. I will let you know if this
works.

Also, with the help of Nathan's feedback I was able to reproduce the
warnings. I have changed the files to remove the warnings and it works.

I will submit another [PATCH v5].

Thank you!
Regards,
Khadija Kamran

> Let me know if this procedure works.
>
> Fabio
>
>

2023-03-16 12:04:01

by Khadija Kamran

[permalink] [raw]
Subject: Re: [PATCH] staging: axis-fifo: initialize timeouts in probe only

On Thu, Mar 16, 2023 at 12:13:23PM +0100, Fabio M. De Francesco wrote:
> Khadija,
>
> I'm not yet sure whether or not you are doing well with reserving 13GB to a
> VM. First of all you should better use multiples of 2GB. Furthermore it looks
> too much memory unless you have a total "real" RAM in your host exceeding
> 18GB. You must be sure you are leaving enough room for the Windows host to run
> smoothly and avoid too much swapping to/from disk.
>

Hey Fabio,

I have 16GB RAM. I just chose the highest option that VMware
recommended. My Windows is running smoothly.

> At the same time I doubt that you can reserve 8 logical cores (2x4) for
> running the VM on VMWare.
>
> However, we have time to investigate and adjust your configuration (if it
> really needs to be fine tuned). I'll try to reach you on IRC, open a private
> window, and assist with this task ASAP.
>
> At the moment I'd prefer to see you focusing on getting your first patch
> accepted by Greg.
>

I am working on the new [PATCH v5] as I was able to remove the warnings.
However, If you still think that we should work on the configurations
then we can start working on it. :)

Thank you for all the help.

Regards,
Khadija



> Thanks,
>
> Fabio
>
>
>