| Message ID | 20230719082755.3399424-1-danishanwar@ti.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce ICSSG based ethernet Driver | expand |
The patch is too big to review. Please break it apart separating into individual features, targeting around 10 patches in the series. That will make it easier for reviewers to take a look at the features in which they have expertise. See two things which jumped out at me immediately below: On Wed, 19 Jul 2023 13:57:55 +0530 MD Danish Anwar wrote: > + ICSSG_STATS(rx_crc_error_frames), > + ICSSG_STATS(rx_max_size_error_frames), > + ICSSG_STATS(rx_frame_min_size), > + ICSSG_STATS(rx_min_size_error_frames), > + ICSSG_STATS(rx_overrun_frames), > + ICSSG_STATS(rx_64B_frames), > + ICSSG_STATS(rx_bucket1_frames), > + ICSSG_STATS(rx_bucket2_frames), > + ICSSG_STATS(rx_bucket3_frames), > + ICSSG_STATS(rx_bucket4_frames), > + ICSSG_STATS(rx_bucket5_frames), > + ICSSG_STATS(rx_total_bytes), > + ICSSG_STATS(rx_tx_total_bytes), > + /* Tx */ > + ICSSG_STATS(tx_good_frames), > + ICSSG_STATS(tx_broadcast_frames), > + ICSSG_STATS(tx_multicast_frames), > + ICSSG_STATS(tx_odd_nibble_frames), > + ICSSG_STATS(tx_underflow_errors), > + ICSSG_STATS(tx_frame_max_size), > + ICSSG_STATS(tx_max_size_error_frames), > + ICSSG_STATS(tx_frame_min_size), > + ICSSG_STATS(tx_min_size_error_frames), > + ICSSG_STATS(tx_bucket1_size), > + ICSSG_STATS(tx_bucket2_size), > + ICSSG_STATS(tx_bucket3_size), > + ICSSG_STATS(tx_bucket4_size), > + ICSSG_STATS(tx_64B_frames), > + ICSSG_STATS(tx_bucket1_frames), > + ICSSG_STATS(tx_bucket2_frames), > + ICSSG_STATS(tx_bucket3_frames), > + ICSSG_STATS(tx_bucket4_frames), > + ICSSG_STATS(tx_bucket5_frames), > + ICSSG_STATS(tx_total_bytes), Please use standard stats: https://docs.kernel.org/next/networking/statistics.html And do not duplicate those stats in the ethool -S output. > +static const char emac_ethtool_priv_flags[][ETH_GSTRING_LEN] = { > + "iet-frame-preemption", > + "iet-mac-verify", > +}; What are these? We have a proper ethtool API for frame preemption.
Hi Jakub, On 20/07/23 10:05 am, Jakub Kicinski wrote: > The patch is too big to review. > > Please break it apart separating into individual features, targeting > around 10 patches in the series. That will make it easier for reviewers > to take a look at the features in which they have expertise. > Sure Jakub. I will try to break this patch in multiple patches as below. Patch 1: Introduce Firmware mapping for the driver (icss_switch_map.h) Patch 2: Introduce mii helper APIs. (icssg_mii_rt.h and icssg_mii_cfg.h). This patch will also introduce basic prueth and emac structures in icssg_prueth.h as these structures will be used by the helper APIs. Patch 3: Introduce firmware configuration and classification APIs. (icssg_classifier.c, icssg_config.h and icssg_config.c) Patch 4: Introduce APIs for ICSSG Queues (icssg_queues.c) Patch 5: Introduce ICSSG Ethernet driver. (icssg_prueth.c and icssg_prueth.h) This patch will enable the driver and basic functionality can work after this patch. This patch will be using all the APIs introduced earlier. This patch will also include Kconfig and Makefile changes. Patch 6: Enable standard statistics via ndo_get_stats64 Patch 7: Introduce ethtool ops for ICSSG Patch 8: Introduce power management support (suspend / resume APIs) However this structure of patches will introduce some APIs earlier (in patch 2,3 and 4) which will be used later by patch 5. I hope it will be OK to introduce APIs and macros earlier and use them later. This restructuring will shorten all the individual patches. However patch 5 will still be a bit large as patch 5 introduces all the neccessary APIs as driver probe / remove, ndo open / close, tx/rx etc. Currnetly this single patch has close to 4000 insertion and is touching 12 files. After restructring patch 5 will have around 1800 insertions and will touch only 4 files (icssg_prueth.c, icssg_prueth.h, Kconfig, Makefile). This is still significant improvement. Please let me know if this is OK. Also this patch has Reviewed-By tag of Andrew. Can I carry forward his Reviewed-By tag in all patches or do I need to drop it? > See two things which jumped out at me immediately below: > > On Wed, 19 Jul 2023 13:57:55 +0530 MD Danish Anwar wrote: >> + ICSSG_STATS(rx_crc_error_frames), > >> + ICSSG_STATS(rx_max_size_error_frames), >> + ICSSG_STATS(rx_frame_min_size), >> + ICSSG_STATS(rx_min_size_error_frames), >> + ICSSG_STATS(rx_overrun_frames), > >> + ICSSG_STATS(rx_64B_frames), >> + ICSSG_STATS(rx_bucket1_frames), >> + ICSSG_STATS(rx_bucket2_frames), >> + ICSSG_STATS(rx_bucket3_frames), >> + ICSSG_STATS(rx_bucket4_frames), >> + ICSSG_STATS(rx_bucket5_frames), >> + ICSSG_STATS(rx_total_bytes), >> + ICSSG_STATS(rx_tx_total_bytes), >> + /* Tx */ >> + ICSSG_STATS(tx_good_frames), >> + ICSSG_STATS(tx_broadcast_frames), >> + ICSSG_STATS(tx_multicast_frames), >> + ICSSG_STATS(tx_odd_nibble_frames), >> + ICSSG_STATS(tx_underflow_errors), >> + ICSSG_STATS(tx_frame_max_size), >> + ICSSG_STATS(tx_max_size_error_frames), >> + ICSSG_STATS(tx_frame_min_size), >> + ICSSG_STATS(tx_min_size_error_frames), >> + ICSSG_STATS(tx_bucket1_size), >> + ICSSG_STATS(tx_bucket2_size), >> + ICSSG_STATS(tx_bucket3_size), >> + ICSSG_STATS(tx_bucket4_size), >> + ICSSG_STATS(tx_64B_frames), >> + ICSSG_STATS(tx_bucket1_frames), >> + ICSSG_STATS(tx_bucket2_frames), >> + ICSSG_STATS(tx_bucket3_frames), >> + ICSSG_STATS(tx_bucket4_frames), >> + ICSSG_STATS(tx_bucket5_frames), >> + ICSSG_STATS(tx_total_bytes), > > Please use standard stats: > https://docs.kernel.org/next/networking/statistics.html > Sure. I will use standard stats in patch 6. > And do not duplicate those stats in the ethool -S output. > Sure I will make sure to not duplicate standard stats in driver specific stats of ethtool -S output. >> +static const char emac_ethtool_priv_flags[][ETH_GSTRING_LEN] = { >> + "iet-frame-preemption", >> + "iet-mac-verify", >> +}; > > What are these? We have a proper ethtool API for frame preemption. I will drop this. Please let me know if this approach looks ok. I will go ahead and start working on it. I Will send next revision at the earliest.
On Thu, 20 Jul 2023 17:12:50 +0530 Md Danish Anwar wrote: > Patch 1: Introduce Firmware mapping for the driver (icss_switch_map.h) > > Patch 2: Introduce mii helper APIs. (icssg_mii_rt.h and icssg_mii_cfg.h). This > patch will also introduce basic prueth and emac structures in icssg_prueth.h as > these structures will be used by the helper APIs. > > Patch 3: Introduce firmware configuration and classification APIs. > (icssg_classifier.c, icssg_config.h and icssg_config.c) > > Patch 4: Introduce APIs for ICSSG Queues (icssg_queues.c) > > Patch 5: Introduce ICSSG Ethernet driver. (icssg_prueth.c and icssg_prueth.h) > This patch will enable the driver and basic functionality can work after this > patch. This patch will be using all the APIs introduced earlier. This patch > will also include Kconfig and Makefile changes. > > Patch 6: Enable standard statistics via ndo_get_stats64 > > Patch 7: Introduce ethtool ops for ICSSG > > Patch 8: Introduce power management support (suspend / resume APIs) > > However this structure of patches will introduce some APIs earlier (in patch > 2,3 and 4) which will be used later by patch 5. I hope it will be OK to > introduce APIs and macros earlier and use them later. > > This restructuring will shorten all the individual patches. However patch 5 > will still be a bit large as patch 5 introduces all the neccessary APIs as > driver probe / remove, ndo open / close, tx/rx etc. > > Currnetly this single patch has close to 4000 insertion and is touching 12 > files. After restructring patch 5 will have around 1800 insertions and will > touch only 4 files (icssg_prueth.c, icssg_prueth.h, Kconfig, Makefile). This is > still significant improvement. > > Please let me know if this is OK. SGTM, thanks! One patch still being larger than others is a bit inevitable. > Also this patch has Reviewed-By tag of Andrew. Can I carry forward his > Reviewed-By tag in all patches or do I need to drop it? If the code is identical I reckon you can carry it.
Hi Danish, On 20/07/2023 14:42, Md Danish Anwar wrote: > Hi Jakub, > > On 20/07/23 10:05 am, Jakub Kicinski wrote: >> The patch is too big to review. >> >> Please break it apart separating into individual features, targeting >> around 10 patches in the series. That will make it easier for reviewers >> to take a look at the features in which they have expertise. >> > > Sure Jakub. I will try to break this patch in multiple patches as below. > > Patch 1: Introduce Firmware mapping for the driver (icss_switch_map.h) > > Patch 2: Introduce mii helper APIs. (icssg_mii_rt.h and icssg_mii_cfg.h). This > patch will also introduce basic prueth and emac structures in icssg_prueth.h as > these structures will be used by the helper APIs. > > Patch 3: Introduce firmware configuration and classification APIs. > (icssg_classifier.c, icssg_config.h and icssg_config.c) > > Patch 4: Introduce APIs for ICSSG Queues (icssg_queues.c) > > Patch 5: Introduce ICSSG Ethernet driver. (icssg_prueth.c and icssg_prueth.h) > This patch will enable the driver and basic functionality can work after this > patch. This patch will be using all the APIs introduced earlier. This patch > will also include Kconfig and Makefile changes. DT binding documentation patch can come here. > > Patch 6: Enable standard statistics via ndo_get_stats64 > > Patch 7: Introduce ethtool ops for ICSSG > > Patch 8: Introduce power management support (suspend / resume APIs) > <snip>
On 21/07/23 1:11 am, Roger Quadros wrote: > Hi Danish, > > On 20/07/2023 14:42, Md Danish Anwar wrote: >> Hi Jakub, >> >> On 20/07/23 10:05 am, Jakub Kicinski wrote: >>> The patch is too big to review. >>> >>> Please break it apart separating into individual features, targeting >>> around 10 patches in the series. That will make it easier for reviewers >>> to take a look at the features in which they have expertise. >>> >> >> Sure Jakub. I will try to break this patch in multiple patches as below. >> >> Patch 1: Introduce Firmware mapping for the driver (icss_switch_map.h) >> >> Patch 2: Introduce mii helper APIs. (icssg_mii_rt.h and icssg_mii_cfg.h). This >> patch will also introduce basic prueth and emac structures in icssg_prueth.h as >> these structures will be used by the helper APIs. >> >> Patch 3: Introduce firmware configuration and classification APIs. >> (icssg_classifier.c, icssg_config.h and icssg_config.c) >> >> Patch 4: Introduce APIs for ICSSG Queues (icssg_queues.c) >> >> Patch 5: Introduce ICSSG Ethernet driver. (icssg_prueth.c and icssg_prueth.h) >> This patch will enable the driver and basic functionality can work after this >> patch. This patch will be using all the APIs introduced earlier. This patch >> will also include Kconfig and Makefile changes. > > DT binding documentation patch can come here. > Sure, Roger. I will add DT binding documentation patch here. >> >> Patch 6: Enable standard statistics via ndo_get_stats64 >> >> Patch 7: Introduce ethtool ops for ICSSG >> >> Patch 8: Introduce power management support (suspend / resume APIs) >> > > <snip> >