Hi Matan, On Tue, Sep 05, 2017 at 01:36:13PM +0000, Matan Azrad wrote: > Hi Adrien > > > -----Original Message----- > > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > > Sent: Tuesday, September 5, 2017 3:02 PM > > To: Matan Azrad <ma...@mellanox.com> > > Cc: Nélio Laranjeiro <nelio.laranje...@6wind.com>; dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v3] net/mlx5: support device removal event > > > > Hi Matan, > > > > On Tue, Sep 05, 2017 at 10:38:21AM +0000, Matan Azrad wrote: > > > Hi Adrien > > > > > > > -----Original Message----- > > > > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > > > > Sent: Tuesday, September 5, 2017 12:28 PM > > > > To: Matan Azrad <ma...@mellanox.com> > > > > Cc: Nélio Laranjeiro <nelio.laranje...@6wind.com>; dev@dpdk.org > > > > Subject: Re: [dpdk-dev] [PATCH v3] net/mlx5: support device removal > > > > event > > > > > > > > Hi Matan, > > > > > > > > On Mon, Sep 04, 2017 at 05:52:55PM +0000, Matan Azrad wrote: > > > > > Hi Adrien, > > > > > > > > > > > -----Original Message----- > > > > > > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > > > > > > Sent: Monday, September 4, 2017 6:33 PM > > > > > > To: Matan Azrad <ma...@mellanox.com> > > > > > > Cc: Nélio Laranjeiro <nelio.laranje...@6wind.com>; dev@dpdk.org > > > > > > Subject: Re: [dpdk-dev] [PATCH v3] net/mlx5: support device > > > > > > removal event > > > > > > > > > > > > Hi Matan, > > > > > > > > > > > > One comment I have is, while this patch adds support for RMV, it > > > > > > also silently addresses a bug (see large comment you added to > > > > > > priv_link_status_update()). > > > > > > > > > > > > This should be split in two commits, with the fix part coming > > > > > > first and CC sta...@dpdk.org, and a second commit adding RMV > > > > > > support > > > > proper. > > > > > > > > > > > > > > > > Actually, the mlx4 bug was not appeared in the mlx5 previous code, > > > > > Probably because the RMV interrupt was not implemented in mlx5 > > > > > before > > > > this patch. > > > > > > > > Good point, no RMV could occur before it is implemented, however a > > > > dedicated commit for the fix itself (i.e. alarm callback not > > > > supposed to end up calling ibv_get_async_event()) might better > > > > explain the logic behind these changes. What I mean is, if there was > > > > no problem, you wouldn't need to make > > > > priv_link_status_update() a separate function, right? > > > > > > > > > > The separation was done mainly because of the new interrupt > > > implementation, else, there was bug here. > > > The unnecessary alarm ibv_get_async_event calling was harmless in the > > > previous code. > > > I gets your point for the logic explanation behind these changes and I > > > can add it in this patch commit log to be clearer, something like: > > > The link update operation was separated from the interrupt callback to > > > avoid RMV interrupt disregard and unnecessary event acknowledgment > > > caused by the inconsistent link status alarm callback. > > > > Yes, it's better to explain why you did this in the commit log, but see > > below. > > > > > > > The big comment just explains the link inconsistent issue and was > > > > > added here since Nelio and I think the new function, > > > > > priv_link_status_update(), justifies this comment for future review. > > > > > > > > I understand, this could also have been part of the commit log of > > > > the dedicated commit. > > > > > > > Are you sure we need to describe the code comment reason in the commit > > log? > > > > It's a change you did to address a possible bug otherwise so we have to, > > however remember that a commit should, as much as possible, do exactly > > one thing. If you need to explain that you did this in order to do that, > > "this" > > and "that" can often be identified as two separate commits. Doing so makes > > it much easier for reviewers to understand the reasoning behind changes > > and leads to quicker reviews (makes instant-acks even possible). > > > > It'd still like a separate commit if you don't mind. > > Sorry, but I think it is an infinite order. > I have just added RMV interrupt, I did a lot of things in this patch for it. > I think I don't need to separate each thing done for this support. > I prefer to stay it in one patch if you don't mind.
I understand that's a lot of work, so let's cut the talk. Since I'm the one requesting for patches to be split, I'll offer to re-spin yours and submit the result as v4, is that OK? -- Adrien Mazarguil 6WIND
