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. > > -- > Adrien Mazarguil > 6WIND
