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.sh...@microchip.com>
> ---
> 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
>
