Message-ID: <20210521232525.GA79835@rocinante.localdomain>
Date:   Sat, 22 May 2021 01:25:25 +0200
From:   Krzysztof Wilczyński <kw@...ux.com>
To:     Shradha Todi <shradha.t@...sung.com>
Cc:     linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        jingoohan1@...il.com, gustavo.pimentel@...opsys.com,
        lorenzo.pieralisi@....com, robh@...nel.org, bhelgaas@...gle.com,
        pankaj.dubey@...sung.com, p.rajanbabu@...sung.com,
        hari.tv@...sung.com, niyas.ahmed@...sung.com, l.mehra@...sung.com
Subject: Re: [PATCH 2/3] PCI: debugfs: Add support for RAS framework in DWC

Hi Shradha,

Thank you for this working on this!  Looks very nice!

A few suggestions below.

> +config PCIE_DW_DEBUGFS
> +	bool "DWC PCIe debugfs entries"
> +	default n

No need to add this default for "no" answer, and this is the Kconfig's
default, so you can omit it here.

> +static int open(struct inode *inode, struct file *file)
> +{
> +	file->private_data = inode->i_private;
> +
> +	return 0;
> +}

Why custom callback?  This seem to almost copy what simple_open() does,
as per:

  int simple_open(struct inode *inode, struct file *file)
  {
  	if (inode->i_private)
  		file->private_data = inode->i_private;
  	return 0;
  }

You seem to be using simple_open() everywhere else.

> + * set_event_number: Function to set event number based on filename
> + *
> + * This function is called from the common read and write function
> + * written for all event counters. Using the debugfs filname, the
> + * group number and event number for the counter is extracted and
> + * then programmed into the control register.
> + *
> + * @file: file pointer to the debugfs entry
> + *
> + * Return: void
> + */

About this and other functions comments - did you intend to format this
and the others as kernel-doc compliant? At the first glance it does look
very similar, as per:

  https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html

[...]
> +/*
> + * ras_event_counter_en_read: Function to get if counter is enable
> + *
> + * This function is invoked when the following command is made:
> + * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/counter_enable

You don't really need to explain when the read() function evokes.  This
is also true for other such comments.

> + * It returns whether the counter is enabled or not

Nit pick: missing period at the end of the sentence (also true in other
code comments and sentences throughout).

[...]
> +	u32 ret;
> +	u32 val;
> +	u32 lane;
> +	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +	ret = kstrtou32_from_user(buf, count, 0, &lane);
> +	if (ret)
> +		return ret;
> +
> +	set_event_number(file);

You could add a newline here so that this is easier to read and also
consistent with other functions.

[...]
> +	err_num = get_error_inj_number(file);
> +	inj_num = (err_num & 0xFF);

Surplus parenthesis above.  This could also be moved to the top where
you declare all the variables.

[...]
> +	err_num = get_error_inj_number(file);
> +	inj_num = (err_num & 0xFF);
> +	err_num = (err_num >> 8);

Surplus parenthesis here too.  Also, you could probably move some of
these to the top, if possible.

[...]
> +static const struct file_operations rx_valid_fops = {
> +	.open = open,
> +	.read = rx_valid_read,
> +	.write = lane_selection_write

Just to make sure - this "lane_selection_write" here is intended?

[...]
> +	if (!pci) {
> +		pr_err("pcie struct is NULL\n");
> +		return -ENODEV;
> +	}

I think, given this particular case should the "pci" be a NULL pointer,
then we ought to have a much worse problems, if you think about this.

Thus, I am not sure if returning here would be better over letting
things crash properly (which might not be ideal either), as if at this
point "pci" is invalid and we still use it here, then something is
potentially has gone really bad somewhere.

Regardless of the approach here, the pr_err() message is not very
helpful in its current wording.

> +
> +	dev = pci->dev;

You can move this to the top where you declare your variable, so that
you would define it at the same time, for example:

  struct device *dev = pci->dev;

[...]
> +	/* Create main directory for each platform driver */
> +	dir = debugfs_create_dir(dirname, NULL);
> +	if (dir == NULL) {
> +		pr_err("error creating directory: %s\n", dirname);
> +		return -ENODEV;
> +	}

A small suggestion about the above: you could perhaps rely on the
following approach:

  if (IS_ERR(dir)) {
  	dev_err(dev, ...);
  	return PTR_ERR(dir);
  }

Unless you want to return -ENODEV for everything, regardless of what the
underlying error code might have been (such as i.e., -EPERM, for
example).  If so, then perhaps the IS_ERR_OR_NULL() macro could be
useful?

Note that debugfs_create_dir() and many other debugfs functions
correctly return -ENODEV if debugfs is not enabled.

Having said that, perhaps this approach would be an overkill.

> +	/* Create sub dirs for Debug, Error injection, Statistics */
> +	ras_des_debug_regs = debugfs_create_dir("ras_des_debug_regs", dir);
> +	if (ras_des_debug_regs == NULL) {
> +		pr_err("error creating directory: %s\n", dirname);
> +		ret = -ENODEV;
> +		goto remove_debug_file;
> +	}
> +
> +	ras_des_error_inj = debugfs_create_dir("ras_des_error_inj", dir);
> +	if (ras_des_error_inj == NULL) {
> +		pr_err("error creating directory: %s\n", dirname);
> +		ret = -ENODEV;
> +		goto remove_debug_file;
> +	}
> +
> +	ras_des_event_counter = debugfs_create_dir("ras_des_counter", dir);
> +	if (ras_des_event_counter == NULL) {
> +		pr_err("error creating directory: %s\n", dirname);
> +		ret = -ENODEV;
> +		goto remove_debug_file;
> +	}

I believe you don't necessary need to print an error message for each
sub-directory created under the root directory "dir".  Why? Technically,
if you can create the root, then you should be able to create all the
sub-directories without issues.

Also, each of the subsequent error messages would print the same name
being the root directory "dir" regardless of which one has failed to be
created.

[...]
> +	/* Create debugfs files for Debug subdirectory */
> +	lane_detection = debugfs_create_file("lane_detection", 0644,
> +					     ras_des_debug_regs, pci,
> +					     &lane_detection_fops);
> +
> +	rx_valid = debugfs_create_file("rx_valid", 0644,
> +					     ras_des_debug_regs, pci,
> +					     &lane_detection_fops);
> +
> +	/* Create debugfs files for Error injection sub dir */

Nit pick: to be consistent, if you could keep using "subdirectory"
everywhere where you use "sub dir".

[...]
> +int create_debugfs_files(struct dw_pcie *pci)
> +{
> +	/* No OP */
> +	return 0;
> +}
> +
> +void remove_debugfs_files(void)
> +{
> +	/* No OP */
> +}
[...]

No need for the "No OP" comment.  Also, you could potentially fit
everything on a single line, for example:

  int create_debugfs_files(struct dw_pcie *pci) { return 0; }
  void remove_debugfs_files(void) {}

But this is a matter of style, so I leave it up to you.

Having said that, both of these functions could use less generic names,
so that any potential current or future symbol name clash would be
avoided, especially since these have global scope.  Thus, adding
a prefix such as e.g., "ras_", or "dw_", etc., I am not sure which one
would be more appropriate.

	Krzysztof

Powered by blists - more mailing lists