[PATCH 0/3] drivers: nubus: Fix coding style issues in nubus.c

[PATCH 0/3] drivers: nubus: Fix coding style issues in nubus.c

[Sayyad Abid]

This patch series addresses coding style improvements in
the Nubus subsystem, specifically in `nubus.c`. These changes
aim to enhance readability and maintainability of the code.

These coding style inconsistencies were found using checkpatch.pl

Changes include:
 1. Improved comment block formatting by aligning `*` in
    multi-line comments.
 2. Fixing assignments inside conditional statements to improve clarity.
 3. Correcting the use of tabs for indentation in specific functions.

Each commit focuses on a specific aspect, as detailed below.


Sayyad Abid (3):
  Fix use of tabs in nubus_get_vendorinfo and nubus_add_board
  Fix use of assignment in if condition in nubus_add_board()
  Fix use of * in comment block in nubus.c

 drivers/nubus/nubus.c | 94 ++++++++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 41 deletions(-)

--
2.39.5

[PATCH 1/3] drivers: nubus: Fix use of tabs in nubus_get_vendorinfo and nubus_add_board in nubus.c

[Sayyad Abid]

This change maintinas code consistency and compliance with
rest of the kernel codebase by implementing use of tabs for
indent where possible

Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
---
 drivers/nubus/nubus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nubus/nubus.c b/drivers/nubus/nubus.c
index ab0f32b901c8..08cf585cb471 100644
--- a/drivers/nubus/nubus.c
+++ b/drivers/nubus/nubus.c
@@ -615,7 +615,7 @@ static int __init nubus_get_vendorinfo(struct nubus_board *board,
 	struct nubus_dir dir;
 	struct nubus_dirent ent;
 	static char *vendor_fields[6] = { "ID", "serial", "revision",
-	                                  "part", "date", "unknown field" };
+					  "part", "date", "unknown field" };

 	pr_debug("    vendor info:\n");
 	nubus_get_subdir(parent, &dir);
@@ -783,7 +783,7 @@ static void __init nubus_add_board(int slot, int bytelanes)
 	/* Set up the directory pointer */
 	board->directory = board->fblock;
 	nubus_move(&board->directory, nubus_expand32(board->doffset),
-	           board->lanes);
+		   board->lanes);

 	nubus_get_root_dir(board, &dir);

--
2.39.5

[PATCH 2/3] drivers: nubus: Fix use of assignment in if condition in nubus_add_board() in nubus.c

[Sayyad Abid]

This change help improve code readabaility by
shifting the assignment statement just above the if statment,
which was earlier inside the condition.
This makes the code clear and easy to read.

Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
---
 drivers/nubus/nubus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nubus/nubus.c b/drivers/nubus/nubus.c
index 08cf585cb471..77da1d14a1db 100644
--- a/drivers/nubus/nubus.c
+++ b/drivers/nubus/nubus.c
@@ -735,7 +735,8 @@ static void __init nubus_add_board(int slot, int bytelanes)
 	nubus_rewind(&rp, FORMAT_BLOCK_SIZE, bytelanes);

 	/* Actually we should probably panic if this fails */
-	if ((board = kzalloc(sizeof(*board), GFP_ATOMIC)) == NULL)
+	board = kzalloc(sizeof(*board), GFP_ATOMIC)
+	if (board == NULL)
 		return;
 	board->fblock = rp;

--
2.39.5

Re: [PATCH 2/3] drivers: nubus: Fix use of assignment in if condition in nubus_add_board() in nubus.c

[Shuah Khan]

On 10/2/24 07:28, Sayyad Abid wrote:
> This change help improve code readabaility by
> shifting the assignment statement just above the if statment,
> which was earlier inside the condition.
> This makes the code clear and easy to read.
> 
> Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
> ---
>   drivers/nubus/nubus.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nubus/nubus.c b/drivers/nubus/nubus.c
> index 08cf585cb471..77da1d14a1db 100644
> --- a/drivers/nubus/nubus.c
> +++ b/drivers/nubus/nubus.c
> @@ -735,7 +735,8 @@ static void __init nubus_add_board(int slot, int bytelanes)
>   	nubus_rewind(&rp, FORMAT_BLOCK_SIZE, bytelanes);
> 
>   	/* Actually we should probably panic if this fails */
> -	if ((board = kzalloc(sizeof(*board), GFP_ATOMIC)) == NULL)
> +	board = kzalloc(sizeof(*board), GFP_ATOMIC)
> +	if (board == NULL)
>   		return;

The code is consistent with coding guidelines. There is no
need to change it.

>   	board->fblock = rp;
> 
> --
> 2.39.5
> 

thanks,
-- Shuah

Re: [PATCH 2/3] drivers: nubus: Fix use of assignment in if condition in nubus_add_board() in nubus.c

[kernel test robot]

Hi Sayyad,

kernel test robot noticed the following build errors:

[auto build test ERROR on gerg-m68knommu/for-next]
[also build test ERROR on linus/master v6.12-rc1 next-20241003]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sayyad-Abid/drivers-nubus-Fix-use-of-tabs-in-nubus_get_vendorinfo-and-nubus_add_board-in-nubus-c/20241002-214920
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gerg/m68knommu.git for-next
patch link:    https://lore.kernel.org/r/20241002132820.402583-3-sayyad.abid16%40gmail.com
patch subject: [PATCH 2/3] drivers: nubus: Fix use of assignment in if condition in nubus_add_board() in nubus.c
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20241003/202410031850.v2wyYvLv-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241003/202410031850.v2wyYvLv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410031850.v2wyYvLv-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/nubus/nubus.c: In function 'nubus_add_board':
>> drivers/nubus/nubus.c:739:9: error: expected ';' before 'if'
     739 |         if (board == NULL)
         |         ^~

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [m]:
   - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]


vim +739 drivers/nubus/nubus.c

   723	
   724	static void __init nubus_add_board(int slot, int bytelanes)
   725	{
   726		struct nubus_board *board;
   727		unsigned char *rp;
   728		unsigned long dpat;
   729		struct nubus_dir dir;
   730		struct nubus_dirent ent;
   731		int prev_resid = -1;
   732	
   733		/* Move to the start of the format block */
   734		rp = nubus_rom_addr(slot);
   735		nubus_rewind(&rp, FORMAT_BLOCK_SIZE, bytelanes);
   736	
   737		/* Actually we should probably panic if this fails */
   738		board = kzalloc(sizeof(*board), GFP_ATOMIC)
 > 739		if (board == NULL)
   740			return;
   741		board->fblock = rp;
   742	
   743		/* Dump the format block for debugging purposes */
   744		pr_debug("Slot %X, format block at 0x%p:\n", slot, rp);
   745		pr_debug("%08lx\n", nubus_get_rom(&rp, 4, bytelanes));
   746		pr_debug("%08lx\n", nubus_get_rom(&rp, 4, bytelanes));
   747		pr_debug("%08lx\n", nubus_get_rom(&rp, 4, bytelanes));
   748		pr_debug("%02lx\n", nubus_get_rom(&rp, 1, bytelanes));
   749		pr_debug("%02lx\n", nubus_get_rom(&rp, 1, bytelanes));
   750		pr_debug("%08lx\n", nubus_get_rom(&rp, 4, bytelanes));
   751		pr_debug("%02lx\n", nubus_get_rom(&rp, 1, bytelanes));
   752		pr_debug("%02lx\n", nubus_get_rom(&rp, 1, bytelanes));
   753		rp = board->fblock;
   754	
   755		board->slot = slot;
   756		board->slot_addr = (unsigned long)nubus_slot_addr(slot);
   757		board->doffset = nubus_get_rom(&rp, 4, bytelanes);
   758		/* rom_length is *supposed* to be the total length of the
   759		 * ROM.  In practice it is the "amount of ROM used to compute
   760		 * the CRC."  So some jokers decide to set it to zero and
   761		 * set the crc to zero so they don't have to do any math.
   762		 * See the Performa 460 ROM, for example.  Those Apple "engineers".
   763		 */
   764		board->rom_length = nubus_get_rom(&rp, 4, bytelanes);
   765		board->crc = nubus_get_rom(&rp, 4, bytelanes);
   766		board->rev = nubus_get_rom(&rp, 1, bytelanes);
   767		board->format = nubus_get_rom(&rp, 1, bytelanes);
   768		board->lanes = bytelanes;
   769	
   770		/* Directory offset should be small and negative... */
   771		if (!(board->doffset & 0x00FF0000))
   772			pr_warn("Slot %X: Dodgy doffset!\n", slot);
   773		dpat = nubus_get_rom(&rp, 4, bytelanes);
   774		if (dpat != NUBUS_TEST_PATTERN)
   775			pr_warn("Slot %X: Wrong test pattern %08lx!\n", slot, dpat);
   776	
   777		/*
   778		 *	I wonder how the CRC is meant to work -
   779		 *		any takers ?
   780		 * CSA: According to MAC docs, not all cards pass the CRC anyway,
   781		 * since the initial Macintosh ROM releases skipped the check.
   782		 */
   783	
   784		/* Set up the directory pointer */
   785		board->directory = board->fblock;
   786		nubus_move(&board->directory, nubus_expand32(board->doffset),
   787			   board->lanes);
   788	
   789		nubus_get_root_dir(board, &dir);
   790	
   791		/* We're ready to rock */
   792		pr_debug("Slot %X resources:\n", slot);
   793	
   794		/* Each slot should have one board resource and any number of
   795		 * functional resources.  So we'll fill in some fields in the
   796		 * struct nubus_board from the board resource, then walk down
   797		 * the list of functional resources, spinning out a nubus_rsrc
   798		 * for each of them.
   799		 */
   800		if (nubus_readdir(&dir, &ent) == -1) {
   801			/* We can't have this! */
   802			pr_err("Slot %X: Board resource not found!\n", slot);
   803			kfree(board);
   804			return;
   805		}
   806	
   807		if (ent.type < 1 || ent.type > 127)
   808			pr_warn("Slot %X: Board resource ID is invalid!\n", slot);
   809	
   810		board->procdir = nubus_proc_add_board(board);
   811	
   812		nubus_get_board_resource(board, slot, &ent);
   813	
   814		while (nubus_readdir(&dir, &ent) != -1) {
   815			struct nubus_rsrc *fres;
   816	
   817			fres = nubus_get_functional_resource(board, slot, &ent);
   818			if (fres == NULL)
   819				continue;
   820	
   821			/* Resources should appear in ascending ID order. This sanity
   822			 * check prevents duplicate resource IDs.
   823			 */
   824			if (fres->resid <= prev_resid) {
   825				kfree(fres);
   826				continue;
   827			}
   828			prev_resid = fres->resid;
   829	
   830			list_add_tail(&fres->list, &nubus_func_rsrcs);
   831		}
   832	
   833		if (nubus_device_register(board))
   834			put_device(&board->dev);
   835	}
   836	

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

[PATCH 3/3] drivers: nubus: Fix use of * in comment block in nubus.c

[Sayyad Abid]

This change help make the comment blocks reader friendly
by adding * on each comment line indicating a continuous
block of comment

Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
---
 drivers/nubus/nubus.c | 87 ++++++++++++++++++++++++-------------------
 1 file changed, 49 insertions(+), 38 deletions(-)

diff --git a/drivers/nubus/nubus.c b/drivers/nubus/nubus.c
index 77da1d14a1db..501c830c3c40 100644
--- a/drivers/nubus/nubus.c
+++ b/drivers/nubus/nubus.c
@@ -24,7 +24,8 @@
 /* Constants */

 /* This is, of course, the size in bytelanes, rather than the size in
-   actual bytes */
+ * actual bytes
+ */
 #define FORMAT_BLOCK_SIZE 20
 #define ROM_DIR_OFFSET 0x24

@@ -42,27 +43,27 @@ module_param_named(populate_procfs, nubus_populate_procfs, bool, 0);
 LIST_HEAD(nubus_func_rsrcs);

 /* Meaning of "bytelanes":
-
-   The card ROM may appear on any or all bytes of each long word in
-   NuBus memory.  The low 4 bits of the "map" value found in the
-   format block (at the top of the slot address space, as well as at
-   the top of the MacOS ROM) tells us which bytelanes, i.e. which byte
-   offsets within each longword, are valid.  Thus:
-
-   A map of 0x0f, as found in the MacOS ROM, means that all bytelanes
-   are valid.
-
-   A map of 0xf0 means that no bytelanes are valid (We pray that we
-   will never encounter this, but stranger things have happened)
-
-   A map of 0xe1 means that only the MSB of each long word is actually
-   part of the card ROM.  (We hope to never encounter NuBus on a
-   little-endian machine.  Again, stranger things have happened)
-
-   A map of 0x78 means that only the LSB of each long word is valid.
-
-   Etcetera, etcetera.  Hopefully this clears up some confusion over
-   what the following code actually does.  */
+ *
+ * The card ROM may appear on any or all bytes of each long word in
+ * NuBus memory.  The low 4 bits of the "map" value found in the
+ *format block (at the top of the slot address space, as well as at
+ *the top of the MacOS ROM) tells us which bytelanes, i.e. which byte
+ * offsets within each longword, are valid.  Thus:
+ *
+ * A map of 0x0f, as found in the MacOS ROM, means that all bytelanes
+ * are valid.
+ *
+ * A map of 0xf0 means that no bytelanes are valid (We pray that we
+ * will never encounter this, but stranger things have happened)
+ *
+ * A map of 0xe1 means that only the MSB of each long word is actually
+ * part of the card ROM.  (We hope to never encounter NuBus on a
+ * little-endian machine.  Again, stranger things have happened)
+ * A map of 0x78 means that only the LSB of each long word is valid.
+ *
+ * Etcetera, etcetera.  Hopefully this clears up some confusion over
+ * what the following code actually does.
+ */

 static inline int not_useful(void *p, int map)
 {
@@ -133,9 +134,10 @@ static void nubus_move(unsigned char **ptr, int len, int map)
 /* Now, functions to read the sResource tree */

 /* Each sResource entry consists of a 1-byte ID and a 3-byte data
-   field.  If that data field contains an offset, then obviously we
-   have to expand it from a 24-bit signed number to a 32-bit signed
-   number. */
+ * field.  If that data field contains an offset, then obviously we
+ * have to expand it from a 24-bit signed number to a 32-bit signed
+ * number.
+ */

 static inline long nubus_expand32(long foo)
 {
@@ -158,14 +160,16 @@ unsigned char *nubus_dirptr(const struct nubus_dirent *nd)
 	unsigned char *p = nd->base;

 	/* Essentially, just step over the bytelanes using whatever
-	   offset we might have found */
+	 * offset we might have found
+	 */
 	nubus_move(&p, nubus_expand32(nd->data), nd->mask);
 	/* And return the value */
 	return p;
 }

 /* These two are for pulling resource data blocks (i.e. stuff that's
-   pointed to with offsets) out of the card ROM. */
+ * pointed to with offsets) out of the card ROM.
+ */

 void nubus_get_rsrc_mem(void *dest, const struct nubus_dirent *dirent,
 			unsigned int len)
@@ -253,7 +257,8 @@ int nubus_get_board_dir(const struct nubus_board *board,
 	dir->mask = board->lanes;

 	/* Now dereference it (the first directory is always the board
-	   directory) */
+	 * directory)
+	 */
 	if (nubus_readdir(dir, &ent) == -1)
 		return -1;
 	if (nubus_get_subdir(&ent, dir) == -1)
@@ -339,8 +344,9 @@ nubus_find_rsrc(struct nubus_dir *dir, unsigned char rsrc_type,
 EXPORT_SYMBOL(nubus_find_rsrc);

 /* Initialization functions - decide which slots contain stuff worth
-   looking at, and print out lots and lots of information from the
-   resource blocks. */
+ * looking at, and print out lots and lots of information from the
+ * resource blocks.
+ */

 static int __init nubus_get_block_rsrc_dir(struct nubus_board *board,
 					   struct proc_dir_entry *procdir,
@@ -542,7 +548,8 @@ nubus_get_functional_resource(struct nubus_board *board, int slot,
 		case NUBUS_RESID_DRVRDIR:
 		{
 			/* MacOS driver.  If we were NetBSD we might
-			   use this :-) */
+			 *  use this :-)
+			 */
 			pr_debug("    driver directory offset: 0x%06x\n",
 				ent.data);
 			nubus_get_block_rsrc_dir(board, dir.procdir, &ent);
@@ -551,8 +558,9 @@ nubus_get_functional_resource(struct nubus_board *board, int slot,
 		case NUBUS_RESID_MINOR_BASEOS:
 		{
 			/* We will need this in order to support
-			   multiple framebuffers.  It might be handy
-			   for Ethernet as well */
+			 * multiple framebuffers.  It might be handy
+			 * for Ethernet as well
+			 */
 			u32 base_offset;

 			nubus_get_rsrc_mem(&base_offset, &ent, 4);
@@ -651,8 +659,9 @@ static int __init nubus_get_board_resource(struct nubus_board *board, int slot,
 		{
 			unsigned short nbtdata[4];
 			/* This type is always the same, and is not
-			   useful except insofar as it tells us that
-			   we really are looking at a board resource. */
+			 * useful except insofar as it tells us that
+			 * we really are looking at a board resource.
+			 */
 			nubus_get_rsrc_mem(nbtdata, &ent, 8);
 			pr_debug("    type: [cat 0x%x type 0x%x sw 0x%x hw 0x%x]\n",
 				nbtdata[0], nbtdata[1], nbtdata[2], nbtdata[3]);
@@ -849,12 +858,14 @@ static void __init nubus_probe_slot(int slot)
 		dp = *rp;

 		/* The last byte of the format block consists of two
-		   nybbles which are "mirror images" of each other.
-		   These show us the valid bytelanes */
+		 * nybbles which are "mirror images" of each other.
+		 * These show us the valid bytelanes
+		 */
 		if ((((dp >> 4) ^ dp) & 0x0F) != 0x0F)
 			continue;
 		/* Check that this value is actually *on* one of the
-		   bytelanes it claims are valid! */
+		 * bytelanes it claims are valid!
+		 */
 		if (not_useful(rp, dp))
 			continue;

--
2.39.5

Re: [PATCH 0/3] drivers: nubus: Fix coding style issues in nubus.c

[Shuah Khan]

On 10/2/24 07:28, Sayyad Abid wrote:
> This patch series addresses coding style improvements in
> the Nubus subsystem, specifically in `nubus.c`. These changes
> aim to enhance readability and maintainability of the code.
> 
> These coding style inconsistencies were found using checkpatch.pl
> 
> Changes include:
>   1. Improved comment block formatting by aligning `*` in
>      multi-line comments.
>   2. Fixing assignments inside conditional statements to improve clarity.
>   3. Correcting the use of tabs for indentation in specific functions.
> 
> Each commit focuses on a specific aspect, as detailed below.
> 
> 
> Sayyad Abid (3):
>    Fix use of tabs in nubus_get_vendorinfo and nubus_add_board
>    Fix use of assignment in if condition in nubus_add_board()
>    Fix use of * in comment block in nubus.c
> 
>   drivers/nubus/nubus.c | 94 ++++++++++++++++++++++++-------------------
>   1 file changed, 53 insertions(+), 41 deletions(-)
> 
> --
> 2.39.5
> 

Why are you sending 3 patches for these changes in one single
file?

thanks,
-- Shuah

Re: [PATCH 0/3] drivers: nubus: Fix coding style issues in nubus.c

[Finn Thain]

Hello sayyad.abid16@gmail.com

Thanks for taking the trouble to send e-mail messages to so many people by 
use of a free e-mail account.

With regards to kernel patches, can I request that you --

1. Don't run checkpatch on existing code
2. Don't send patches that break the build
3. Don't claim to be improving code style by sending patches that violate 
   code style rules
4. Read the mailing lists and study the development process before trying 
   to contribute

Thanks for your consideration.

Regards
Finn

On Wed, 2 Oct 2024, Sayyad Abid wrote:

> This patch series addresses coding style improvements in
> the Nubus subsystem, specifically in `nubus.c`. These changes
> aim to enhance readability and maintainability of the code.
> 
> These coding style inconsistencies were found using checkpatch.pl
> 
> Changes include:
>  1. Improved comment block formatting by aligning `*` in
>     multi-line comments.
>  2. Fixing assignments inside conditional statements to improve clarity.
>  3. Correcting the use of tabs for indentation in specific functions.
> 
> Each commit focuses on a specific aspect, as detailed below.
> 
> 
> Sayyad Abid (3):
>   Fix use of tabs in nubus_get_vendorinfo and nubus_add_board
>   Fix use of assignment in if condition in nubus_add_board()
>   Fix use of * in comment block in nubus.c
> 
>  drivers/nubus/nubus.c | 94 ++++++++++++++++++++++++-------------------
>  1 file changed, 53 insertions(+), 41 deletions(-)
> 
> --
> 2.39.5
> 
> 

Re: [PATCH 0/3] drivers: nubus: Fix coding style issues in nubus.c

[Sayyad Abid]

On Thu, Oct 3, 2024 at 4:25 AM Finn Thain <fthain@linux-m68k.org> wrote:
>
>
> Hello sayyad.abid16@gmail.com
>
> Thanks for taking the trouble to send e-mail messages to so many people by
> use of a free e-mail account.
>
> With regards to kernel patches, can I request that you --
>
> 1. Don't run checkpatch on existing code
> 2. Don't send patches that break the build
> 3. Don't claim to be improving code style by sending patches that violate
>    code style rules
> 4. Read the mailing lists and study the development process before trying
>    to contribute
>
> Thanks for your consideration.
>
> Regards
> Finn
>
> On Wed, 2 Oct 2024, Sayyad Abid wrote:
>
> > This patch series addresses coding style improvements in
> > the Nubus subsystem, specifically in `nubus.c`. These changes
> > aim to enhance readability and maintainability of the code.
> >
> > These coding style inconsistencies were found using checkpatch.pl
> >
> > Changes include:
> >  1. Improved comment block formatting by aligning `*` in
> >     multi-line comments.
> >  2. Fixing assignments inside conditional statements to improve clarity.
> >  3. Correcting the use of tabs for indentation in specific functions.
> >
> > Each commit focuses on a specific aspect, as detailed below.
> >
> >
> > Sayyad Abid (3):
> >   Fix use of tabs in nubus_get_vendorinfo and nubus_add_board
> >   Fix use of assignment in if condition in nubus_add_board()
> >   Fix use of * in comment block in nubus.c
> >
> >  drivers/nubus/nubus.c | 94 ++++++++++++++++++++++++-------------------
> >  1 file changed, 53 insertions(+), 41 deletions(-)
> >
> > --
> > 2.39.5
> >
> >
My sincere apologies for causing the trouble,
I overlooked a few things there,
I will keep a note of your points and make sure
that it is not repeated again.

Thank You!

-- 
Abid