Re: [PATCH 1/2] switchtec: Fix false maximum supported PCIe function number issue

[Bjorn Helgaas]

Hi Wesley,

On Mon, Apr 08, 2019 at 10:34:47PM +0800, Wesley Sheng wrote:
> The hardware supports up to 255 PFFs and the driver only supports 48, so
> this patch updates the driver to support them all.
> To be backward compatible, a new ioctl and corresponding data
> structure are created, while keep the deprecated one.

The above is either one paragraph that needs to be rewrapped, or two
paragraphs that need a blank line between.

What's a PFF?

> Signed-off-by: Wesley Sheng <wesley.sheng@xxxxxxxxxxxxx>
> ---
>  drivers/pci/switch/switchtec.c       | 39 +++++++++++++++++++++++++-----------
>  include/linux/switchtec.h            |  2 +-
>  include/uapi/linux/switchtec_ioctl.h | 13 +++++++++++-
>  3 files changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
> index e22766c..7df9a69 100644
> --- a/drivers/pci/switch/switchtec.c
> +++ b/drivers/pci/switch/switchtec.c
> @@ -658,19 +658,25 @@ static int ioctl_flash_part_info(struct switchtec_dev *stdev,
>  
>  static int ioctl_event_summary(struct switchtec_dev *stdev,
>  	struct switchtec_user *stuser,
> -	struct switchtec_ioctl_event_summary __user *usum)
> +	struct switchtec_ioctl_event_summary __user *usum,
> +	size_t size)
>  {
> -	struct switchtec_ioctl_event_summary s = {0};
> +	struct switchtec_ioctl_event_summary *s;
>  	int i;
>  	u32 reg;
> +	int ret = 0;
>  
> -	s.global = ioread32(&stdev->mmio_sw_event->global_summary);
> -	s.part_bitmap = ioread32(&stdev->mmio_sw_event->part_event_bitmap);
> -	s.local_part = ioread32(&stdev->mmio_part_cfg->part_event_summary);
> +	s = kzalloc(sizeof(*s), GFP_KERNEL);
> +	if (!s)
> +		return -ENOMEM;
> +
> +	s->global = ioread32(&stdev->mmio_sw_event->global_summary);
> +	s->part_bitmap = ioread32(&stdev->mmio_sw_event->part_event_bitmap);
> +	s->local_part = ioread32(&stdev->mmio_part_cfg->part_event_summary);
>  
>  	for (i = 0; i < stdev->partition_count; i++) {
>  		reg = ioread32(&stdev->mmio_part_cfg_all[i].part_event_summary);
> -		s.part[i] = reg;
> +		s->part[i] = reg;
>  	}
>  
>  	for (i = 0; i < SWITCHTEC_MAX_PFF_CSR; i++) {
> @@ -679,15 +685,19 @@ static int ioctl_event_summary(struct switchtec_dev *stdev,
>  			break;
>  
>  		reg = ioread32(&stdev->mmio_pff_csr[i].pff_event_summary);
> -		s.pff[i] = reg;
> +		s->pff[i] = reg;
>  	}
>  
> -	if (copy_to_user(usum, &s, sizeof(s)))
> -		return -EFAULT;
> +	if (copy_to_user(usum, s, size)) {
> +		ret = -EFAULT;
> +		goto error_case;
> +	}
>  
>  	stuser->event_cnt = atomic_read(&stdev->event_cnt);
>  
> -	return 0;
> +error_case:
> +	kfree(s);
> +	return ret;
>  }
>  
>  static u32 __iomem *global_ev_reg(struct switchtec_dev *stdev,
> @@ -977,8 +987,9 @@ static long switchtec_dev_ioctl(struct file *filp, unsigned int cmd,
>  	case SWITCHTEC_IOCTL_FLASH_PART_INFO:
>  		rc = ioctl_flash_part_info(stdev, argp);
>  		break;
> -	case SWITCHTEC_IOCTL_EVENT_SUMMARY:
> -		rc = ioctl_event_summary(stdev, stuser, argp);
> +	case SWITCHTEC_IOCTL_EVENT_SUMMARY_LEGACY:
> +		rc = ioctl_event_summary(stdev, stuser, argp,
> +					 sizeof(struct switchtec_ioctl_event_summary_legacy));
>  		break;
>  	case SWITCHTEC_IOCTL_EVENT_CTL:
>  		rc = ioctl_event_ctl(stdev, argp);
> @@ -989,6 +1000,10 @@ static long switchtec_dev_ioctl(struct file *filp, unsigned int cmd,
>  	case SWITCHTEC_IOCTL_PORT_TO_PFF:
>  		rc = ioctl_port_to_pff(stdev, argp);
>  		break;
> +	case SWITCHTEC_IOCTL_EVENT_SUMMARY:
> +		rc = ioctl_event_summary(stdev, stuser, argp,
> +					 sizeof(struct switchtec_ioctl_event_summary));
> +		break;
>  	default:
>  		rc = -ENOTTY;
>  		break;
> diff --git a/include/linux/switchtec.h b/include/linux/switchtec.h
> index 52a079b..0cfc34a 100644
> --- a/include/linux/switchtec.h
> +++ b/include/linux/switchtec.h
> @@ -20,7 +20,7 @@
>  #include <linux/cdev.h>
>  
>  #define SWITCHTEC_MRPC_PAYLOAD_SIZE 1024
> -#define SWITCHTEC_MAX_PFF_CSR 48
> +#define SWITCHTEC_MAX_PFF_CSR 255
>  
>  #define SWITCHTEC_EVENT_OCCURRED BIT(0)
>  #define SWITCHTEC_EVENT_CLEAR    BIT(0)
> diff --git a/include/uapi/linux/switchtec_ioctl.h b/include/uapi/linux/switchtec_ioctl.h
> index 4f4daf8..c912b5a 100644
> --- a/include/uapi/linux/switchtec_ioctl.h
> +++ b/include/uapi/linux/switchtec_ioctl.h
> @@ -50,7 +50,7 @@ struct switchtec_ioctl_flash_part_info {
>  	__u32 active;
>  };
>  
> -struct switchtec_ioctl_event_summary {
> +struct switchtec_ioctl_event_summary_legacy {
>  	__u64 global;
>  	__u64 part_bitmap;
>  	__u32 local_part;
> @@ -59,6 +59,15 @@ struct switchtec_ioctl_event_summary {
>  	__u32 pff[48];
>  };
>  
> +struct switchtec_ioctl_event_summary {
> +	__u64 global;
> +	__u64 part_bitmap;
> +	__u32 local_part;
> +	__u32 padding;
> +	__u32 part[48];
> +	__u32 pff[255];
> +};
> +
>  #define SWITCHTEC_IOCTL_EVENT_STACK_ERROR		0
>  #define SWITCHTEC_IOCTL_EVENT_PPU_ERROR			1
>  #define SWITCHTEC_IOCTL_EVENT_ISP_ERROR			2
> @@ -127,6 +136,8 @@ struct switchtec_ioctl_pff_port {
>  	_IOWR('W', 0x41, struct switchtec_ioctl_flash_part_info)
>  #define SWITCHTEC_IOCTL_EVENT_SUMMARY \
>  	_IOR('W', 0x42, struct switchtec_ioctl_event_summary)
> +#define SWITCHTEC_IOCTL_EVENT_SUMMARY_LEGACY \
> +	_IOR('W', 0x42, struct switchtec_ioctl_event_summary_legacy)

I'm not an ioctl expert.  Is the different struct size enough to
distinguish these two, since they're both ioctl number 0x42?

If I'm reading the patch right, a user program compiled against the
old header will continue working unchanged (with only 48 PFFs) even
though the kernel struct name changed from
switchtec_ioctl_event_summary to switchtec_ioctl_event_summary_legacy.

But if it is merely recompiled against the new header with no other
change, it will silently start using 255 PFFs.  I guess as long as it
uses "sizeof(switchtec_ioctl_event_summary.pff)" or similar, it should
work, but if it assumed an array size of 48, it will break.

No doubt this is all exactly what you intended, and if I understood
ioctls it would be obvious to me.  But it would be *more* obviously
backwards-compatible if you left the existing ioctl number and
structure the same and merely added a new
"SWITCHTEC_IOCTL_EVENT_SUMMARY_EXTENDED" with a new number and a new
struct.  So I'm just asking to be sure.

>  #define SWITCHTEC_IOCTL_EVENT_CTL \
>  	_IOWR('W', 0x43, struct switchtec_ioctl_event_ctl)
>  #define SWITCHTEC_IOCTL_PFF_TO_PORT \
> -- 
> 2.7.4
>